adriangb opened a new pull request, #10353: URL: https://github.com/apache/arrow-rs/pull/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. # 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 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
