Repository navigation
Conversation
3 of 5 tasks
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting committer review
kszucs
marked this pull request as ready for review
October 5, 2026 10:05
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The public column-writer factory now rejects a previously supported CDC configuration.
Review effort: Balanced
Findings: 1
What changed in this PR
Improves Parquet content-defined chunking by preserving per-column state across row groups and reducing hash-state cache contention.
Changes:
- Moves rolling hash state into a locally processed
GearHash. - Shares per-column chunkers across row groups.
- Adds row-group boundary tests and a threaded benchmark.
| File | Description |
|---|---|
cpp/src/parquet/file_writer.cc |
Creates and shares per-column chunkers. |
cpp/src/parquet/column_writer.h |
Adds the internal shared-chunker factory. |
cpp/src/parquet/column_writer.cc |
Uses borrowed chunkers across row groups. |
cpp/src/parquet/chunker_internal.h |
Adds move operations and options-based construction. |
cpp/src/parquet/chunker_internal.cc |
Processes hash state locally. |
cpp/src/parquet/chunker_internal_test.cc |
Tests row-group-independent boundaries. |
cpp/src/parquet/arrow/reader_writer_benchmark.cc |
Benchmarks threaded and sequential CDC writes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
kszucs
added a commit
that referenced
this pull request
Oct 12, 2026
#51685) ### Rationale for this change The content-defined chunker was owned by the column writer, which is recreated for every row group, so the chunking state was reset at each row group boundary. See #51684. ### What changes are included in this PR? The file writer creates one chunker per leaf column and passes it to the column writers of every row group through a private `ColumnWriter::Make()` overload. ### Are these changes tested? Yes. `IndependentOfRowGroupBoundaries` writes the same table into one and into six row groups and expects the same page boundaries apart from the row group ends. The first commit adds it alone, failing without the fix. ### Are there any user-facing changes? - Files with multiple row groups get different page boundaries, since the chunking continues across row groups. - `ColumnWriter::Make()` is unchanged, but column writers created through it no longer chunk, so content-defined chunking requires `ParquetFileWriter`. - Multi-threaded writes with content-defined chunking are about 1.7× slower until #51692 (9.9 ms vs 5.8 ms writing 12 int32 columns of 1M rows on an M4 Max) ### Deduplication results A 1 GiB file of 128Mi random int64 values (PLAIN, snappy, default CDC options) compared with a copy that has 10 rows inserted at 20%. The ratio is the share of bytes left after deduplicating both files with the Xet chunker; 50% means the edited copy adds almost nothing. | Row groups | pyarrow 25.0.1 | This PR | arrow-rs 58.4 | |---|---|---|---| | 8 × 16Mi rows | 50.2% | 50.1% | 50.1% | | 32 × 4Mi rows | 50.6% | 50.4% | 50.5% | | 128 × 1Mi rows | 52.4% | 51.6% | 51.6% | | 512 × 256Ki rows | 59.8% | 56.4% | 56.5% | | 2048 × 64Ki rows | 84.8% | 74.2% | 74.2% | | Without CDC | 86.3% | 86.3% | 86.3% | The page boundaries now match arrow-rs, which already keeps the chunker state across row groups. ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. Claude Code wrote the code, tests and description under human direction. **PR code and description written by:** - [ ] Human - [x] AI **Reviewed before submission by:** - [x] Human - [x] AI - [ ] Not reviewed 🤖 Generated with [Claude Code](https://claude.com/claude-code) * GitHub Issue: #51684
The rolling hash state was stored to memory for every hashed byte, because the values read through byte pointers may alias it. GetChunks() now works on a local copy of a GearHash and stores it back once, keeping the state in registers: chunking is 1.2 to 1.6 times faster on a single thread and multi-threaded writes about 2 times faster. BM_WriteContentDefinedChunking measures both.
kszucs
force-pushed
the
cdc-rolling-hash
branch
from
October 12, 2026 10:11
0a8f32a to
2f5a101
Compare
github-actions
Bot
added
awaiting changes
Member
Author
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
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.
Rationale for this change
#51685 keeps one content-defined chunker per column for the whole file, so the file writer now creates the chunkers together. The chunker's hash loop stores its rolling hash state to memory for every hashed byte, because the values it reads through byte pointers may alias that state. That slows down the chunking, and with the chunkers next to each other, the threads writing neighbouring columns also contend for their cache lines.
This PR is stacked on #51685, only the last commit is new.
What changes are included in this PR?
GearHashvalue.GetChunks()rolls a local copy of it and stores it back once, so the compiler keeps the state in registers.BM_WriteContentDefinedChunkingbenchmarks writing 16 int32 columns of 1M rows with content-defined chunking, with and without threads.Are these changes tested?
The existing CDC tests cover the chunking, the chunk boundaries don't change. Medians of
BM_WriteContentDefinedChunkingon an Apple M4 Max:Are there any user-facing changes?
No, writing with content-defined chunking gets faster.
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
Claude Code wrote the code, tests and description under human direction.
PR code and description written by:
Reviewed before submission by:
🤖 Generated with Claude Code