Avoid an extra copy in SerializedRowGroupWriter - #10052
Closed
alamb wants to merge 1 commit into
Closed
Conversation
alamb
changed the title
SerializedRowGroupWriter
…void a copy at splice
The Arrow writer buffers each column chunk's encoded pages as a
`Vec<Bytes>` and, at row-group flush, splices them into the output. It
did this by implementing `ChunkReader`/`Read` over the buffer and driving
`std::io::copy`, which copies every byte a second time through an
intermediate stack buffer (and issues one write per ~8 KiB).
Add `SerializedRowGroupWriter::append_column_from_pages`, which takes the
already-encoded column as an `IntoIterator<Item = Bytes>` and writes each
buffer straight to the output with a single `write_all`. The Arrow writer
now uses it, dropping the bespoke `ArrowColumnChunkReader`/`ChunkReader`/
`Length` adapter entirely.
`append_column` (the `ChunkReader`-based public API) is unchanged in
behavior; its splice preamble and the offset-remap epilogue are factored
into shared `begin_appended_column`/`finish_appended_column` helpers that
both paths use. Output is byte-identical to before.
Also fixes the splice length-mismatch error, which used a single-arg
`general_err!` and so printed the literal text `{read_length} got
{write_length}` instead of the values.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alamb
force-pushed
the
alamb/arrow-writer-splice-no-copy
branch
from
June 2, 2026 20:07
a70766d to
b02afd2
Compare
Contributor
Author
|
run benchmark arrow_writer |
Contributor
Author
|
run benchmark parquet_round_trip |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alamb/arrow-writer-splice-no-copy (b02afd2) to 1ae2469 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alamb/arrow-writer-splice-no-copy (b02afd2) to 1ae2469 (merge-base) diff File an issue against this benchmark runner |
alamb
commented
Jun 2, 2026
| .unwrap_or_else(|| metadata.data_page_offset()); | ||
| let src_length = metadata.compressed_size(); | ||
|
|
||
| let write_offset = self.buf.bytes_written(); |
Contributor
Author
There was a problem hiding this comment.
It is kind of hard to see in the github diff, but basically I just refactored the stuff before this call into one function (begin_appended_column) and the stuff after this call into another (finish_appended_column)
The code is almost the same, just moved around and documented better
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark parquet_round_trip |
alamb
marked this pull request as ready for review
June 2, 2026 21:01
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing alamb/arrow-writer-splice-no-copy (b02afd2) to 1ae2469 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
Looks to me like there is small, but reproducible improvement |
Contributor
|
do we still want this PR? |
Contributor
|
I think yes but it needed some rework. I opened #10353 with the suggested path forward. |
alamb
marked this pull request as draft
July 17, 2026 21:13
Contributor
Author
Let's take that path |
Jefffrey
pushed a commit
that referenced
this pull request
Jul 19, 2026
…:copy` (adapts #10052) (#10353) # Which issue does this PR close? - Adapts #10052 (by @alamb) to the current `PageStore`-based writer. That PR predates #10020 / #10142 and no longer applies cleanly; this PR carries its idea forward. Closes #10052's use case. - closes #10052 # Rationale for this change When the `ArrowWriter` flushes a row group, each column chunk's buffered pages are spliced into the output through `StreamingColumnChunkReader` (a `Read`) driven by `std::io::copy`. Every byte is copied twice — once into `io::copy`'s fixed 8 KiB buffer, then again into `TrackedWrite`'s `BufWriter` — in ~`len / 8 KiB` write calls. The page blobs are already contiguous owned `Bytes` sitting in the `PageStore`; we can write each one straight to the sink with a single `write_all` (large writes bypass the `BufWriter` buffer entirely). #10052 did exactly this, but its `IntoIterator<Item = Bytes>` signature can't express the fallible, one-page-at-a-time drain of a (possibly spilling) `PageStore`, and it assumed the pre-#10020 `Vec<Bytes>` chunk representation. # What changes are included in this PR? - `StreamingColumnChunkReader` (`Read` impl) becomes `StreamingColumnChunkPages`, an `Iterator<Item = Result<Bytes>>` that `take()`s each page out of the `PageStore` as it is consumed — preserving the one-page-in-memory bound for spilling backends. - New `pub(crate) SerializedRowGroupWriter::append_column_from_pages`, which writes each page blob with a single `write_all`. - The shared validation / offset-rewriting code is factored into `begin_appended_column` / `finish_appended_column` (same refactor as #10052), also used by the existing `Read`-based path that still backs the public `append_column`. # Are these changes tested? Covered by existing tests. Also verified that the output files are byte-for-byte identical to main for multi-row-group writes with int / string / dictionary columns under both default and small-page properties. Local benchmark results (`arrow_writer` bench, Apple Silicon), reproducing the wins from #10052's benchmark run without its `large_string_non_null` regression: | benchmark | main | this PR | change | |---|---|---|---| | `string_dictionary/default` | 36.9 ms | 27.9 ms | ~24% faster | | `list_primitive/default` | 179.8 ms | 150.1 ms | ~17% faster | | `large_string_non_null/default` | 31.4 ms | 30.1 ms | ~4% faster | # Are there any user-facing changes? No. `append_column_from_pages` is kept `pub(crate)` for now; it could be made public in a follow-up if the `Result<Bytes>` iterator contract is deemed a good public API (#10052 proposed a public variant). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Sd8JjF3qQMkn7htofu8bF3 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
When the Arrow
ArrowWriterflushes a row group, each column chunk's encoded pages (buffered as aVec<Bytes>) are written into the output file. Today that copy goes throughArrowColumnChunkReaderthat implementsRead, driven bystd::io::copy:This means that every byte of every column chunk is copied twice, in ~
len / 8 KiBwrite calls. TheBytesblobs are already contiguous and owned; we can write each straight to the sink with a singlewrite_all.What changes are included in this PR?
Are these changes tested?
Yes, but existig CI
Are there any user-facing changes?
One additive public method (
append_column_from_pages). No breaking changes, and the resulting files are the same byte for bytePartly 🤖 Generated with Claude Code (which is very good at advertising itself)