Repository navigation
GH-51684: [C++][Parquet] Keep the CDC chunking state across row groups - #51685
Conversation
ed7b88a to
10d98d8
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The changed public factory breaks ABI and disables CDC for existing direct callers.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Persists Parquet content-defined chunking state across row groups.
Changes:
- Moves chunker ownership to the file writer.
- Passes chunkers into successive column writers.
- Adds row-group boundary independence coverage.
| File | Description |
|---|---|
cpp/src/parquet/file_writer.cc |
Manages per-column chunkers. |
cpp/src/parquet/column_writer.h |
Extends the column-writer factory. |
cpp/src/parquet/column_writer.cc |
Uses externally managed chunkers. |
cpp/src/parquet/chunker_internal.h |
Updates ownership documentation. |
cpp/src/parquet/chunker_internal.cc |
Stores level metadata by value. |
cpp/src/parquet/chunker_internal_test.cc |
Tests stable page boundaries. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
10d98d8 to
8d016a7
Compare
8d016a7 to
f556bae
Compare
f556bae to
0a0f214
Compare
0a0f214 to
0e77f38
Compare
f20c5f0 to
77b9299
Compare
Drop ContentDefinedChunker::Make() and the move assignment operator, the file writer only needs the move constructor to keep the chunkers in a vector.
| static std::shared_ptr<ColumnWriter> Make( | ||
| ColumnChunkMetaDataBuilder*, std::unique_ptr<PageWriter>, | ||
| const WriterProperties* properties, BloomFilter* bloom_filter = NULLPTR, | ||
| internal::ContentDefinedChunker* content_defined_chunker = NULLPTR); |
There was a problem hiding this comment.
Is it permissible to expose internal class via a public interface and should internal::ContentDefinedChunker be moved out of the internal namespace?
There was a problem hiding this comment.
chunker_internal.h isn't installed (headers matching internal are skipped), so code outside Arrow can only pass null; the forward declaration just lets the file writer pass its chunker. Parquet's public headers already take ::arrow::internal::Executor* the same way. Making the chunker public would commit us to its API, which is out of scope here.
There was a problem hiding this comment.
Well, originally I added another private Make for this exact reason, but then removed :)
Though it might be better idea to keep a private Make which can be removed once the chunker becomes public?
There was a problem hiding this comment.
I moved the chunker to a private ColumnWriter::Make() overload:
- The public
Make()stays unchanged, so there's no API or ABI break. ContentDefinedChunkerlives inchunker_internal.h, which isn't installed, so it shouldn't appear in a public signature that users can't even include.- Once CDC becomes a public feature, the private overload can be removed, and the chunker can be added to the public
Make()with a proper public type.
There was a problem hiding this comment.
I simply noticed this potential issue. Given the precedent ::arrow::internal::Executor, either approach works for me.
There was a problem hiding this comment.
To correct my earlier comparison: ::arrow::internal::Executor is declared in the installed arrow/util/thread_pool.h, so users can create one (e.g. via ThreadPool::Make()); internal there only means it has no stability guarantee. ContentDefinedChunker lives in chunker_internal.h, which isn't installed, so users couldn't construct one at all. That's another reason to keep it in the private overload.
|
Thanks @wgtmac for reviewing! I addressed the issues, ready for another look. |
| static std::shared_ptr<ColumnWriter> Make( | ||
| ColumnChunkMetaDataBuilder*, std::unique_ptr<PageWriter>, | ||
| const WriterProperties* properties, BloomFilter* bloom_filter = NULLPTR, | ||
| internal::ContentDefinedChunker* content_defined_chunker = NULLPTR); |
There was a problem hiding this comment.
Is it permissible to expose internal class via a public interface and should internal::ContentDefinedChunker be moved out of the internal namespace?
The file writer decides from the writer properties whether to pass a chunker, so the column writer doesn't need to check them.
Pass the content defined chunker through a private overload instead, so the internal chunker isn't part of the public API.
|
Thanks everyone for the reviews! Merging. |
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.
IndependentOfRowGroupBoundarieswrites 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?
ColumnWriter::Make()is unchanged, but column writers created through it no longer chunk, so content-defined chunking requiresParquetFileWriter.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.
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, 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