GH-51684: [C++][Parquet] Keep the CDC chunking state across row groups by kszucs · Pull Request #51685 · apache/arrow

kszucs · 2026-10-02T07:22:45Z

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 new ColumnWriter::Make() argument.

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() takes an optional chunker as its last argument. Existing calls still compile, but the ABI changes. With content-defined chunking enabled and no chunker it throws, so such column writers have to come from ParquetFileWriter.
Multi-threaded writes with content-defined chunking are about 1.7× slower until GH-51684: [C++][Parquet] Speed up content-defined chunking with a local gear hash #51692 (9.9 ms vs 5.8 ms writing 12 int32 columns of 1M rows on an M4 Max, 4.4 ms with GH-51684: [C++][Parquet] Speed up content-defined chunking with a local gear hash #51692): the chunkers are now allocated together, so threads writing neighbouring columns contend for their cache lines.

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%
`ParquetWriter.write()` per 100k rows	75.2%	66.7%	64.3%*
Without CDC	86.3%	86.3%	86.3%

* arrow-rs doesn't start a row group per write call, so it writes 8 row groups where pyarrow writes 1343.

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:

Human
AI

Reviewed before submission by:

Human
AI
Not reviewed

🤖 Generated with Claude Code

GitHub Issue: [C++][Parquet] Content-defined chunking resets its state at every row group #51684

Copilot

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)

Preserve Make ABI and content-defined chunker creation · New

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.

Copilot

🔵 Needs a closer look

The new public factory rejection behavior lacks direct regression coverage.

1 open finding

Public factory loses content-defined chunking

1 resolved since last review

Preserve Make ABI and content-defined chunker creation

Previously missed (1)

In code that hasn't changed since last review

Add regression test for CDC-enabled ColumnWriter::Make failure

cpp/src/parquet/column_writer.cc:784

The new public-factory failure path is not covered: the added row-group test only writes through ParquetFileWriter. Add a regression test that calls the existing four-argument ColumnWriter::Make with CDC-enabled properties and asserts this ParquetException, so this intentional public API behavior cannot silently change.

🧠 Review effort: Balanced

The column writer owns the content defined chunker and is recreated for every row group, so the chunking state is reset at each row group boundary. Writing the same table into one and into multiple row groups must give the same page boundaries apart from the row group ends.

The column writer created the chunker for every column chunk, so the chunking state was reset at each row group. The file writer now keeps one chunker per leaf column and passes it to the column writers of all the row groups.

The chunker stores the level information by value, so the file writer doesn't need to share it with the column writers. Computing it is cheap.

… previous column The column ordinal and the column metadata fall out of step if closing the previous column writer throws and NextColumn() is called again, but that isn't specific to content defined chunking.

…lumnWriter::Make() The column writer throws if content defined chunking is enabled without a chunker, so a separate private overload isn't needed.

Copilot

🔵 Needs a closer look

The exported ColumnWriter::Make change still breaks ABI compatibility and existing CDC callers.

1 open finding

Public factory loses content-defined chunking

🧠 Review effort: Balanced

Drop ContentDefinedChunker::Make() and the move assignment operator, the file writer only needs the move constructor to keep the chunkers in a vector.

Copilot

🟡 Changes recommended

The exported ColumnWriter::Make change still breaks API/ABI compatibility.

2 open findings

🧠 Review effort: Balanced

+  static std::shared_ptr<ColumnWriter> Make(
+      ColumnChunkMetaDataBuilder*, std::unique_ptr<PageWriter>,
+      const WriterProperties* properties, BloomFilter* bloom_filter = NULLPTR,
+      internal::ContentDefinedChunker* content_defined_chunker = NULLPTR);


kszucs · 2026-10-10T18:16:03Z

Thanks @wgtmac for reviewing! I addressed the issues, ready for another look.

github-actions Bot added Component: Parquet Component: C++ awaiting committer review Awaiting committer review labels Oct 2, 2026

kszucs force-pushed the cdc-row-group-reset branch 5 times, most recently from ed7b88a to 10d98d8 Compare October 2, 2026 10:14

kszucs marked this pull request as ready for review October 2, 2026 10:31

kszucs requested review from HuaHuaY, pitrou and wgtmac as code owners October 2, 2026 10:31

Copilot AI balanced review requested due to automatic review settings October 2, 2026 10:31

Copilot started reviewing on behalf of kszucs October 2, 2026 10:32 View session

Copilot AI reviewed Oct 2, 2026

View reviewed changes

Comment thread cpp/src/parquet/column_writer.h Outdated

kszucs force-pushed the cdc-row-group-reset branch from 10d98d8 to 8d016a7 Compare October 2, 2026 13:54

Copilot AI balanced review requested due to automatic review settings October 2, 2026 13:54

Copilot started reviewing on behalf of kszucs October 2, 2026 13:56 View session

This comment was marked as duplicate.

Sign in to view

Copilot AI balanced review requested due to automatic review settings October 2, 2026 15:29

kszucs force-pushed the cdc-row-group-reset branch from 8d016a7 to f556bae Compare October 2, 2026 15:29

Copilot started reviewing on behalf of kszucs October 2, 2026 15:29 View session

This comment was marked as duplicate.

Sign in to view

Copilot AI balanced review requested due to automatic review settings October 2, 2026 16:35

kszucs force-pushed the cdc-row-group-reset branch from f556bae to 0a0f214 Compare October 2, 2026 16:35

Copilot started reviewing on behalf of kszucs October 2, 2026 16:35 View session

This comment was marked as duplicate.

Sign in to view

Copilot AI balanced review requested due to automatic review settings October 2, 2026 16:38

kszucs force-pushed the cdc-row-group-reset branch from 0a0f214 to 0e77f38 Compare October 2, 2026 16:38

Copilot started reviewing on behalf of kszucs October 2, 2026 16:38 View session

wgtmac reviewed Oct 10, 2026

View reviewed changes

Comment thread cpp/src/parquet/column_writer.h Outdated

Comment thread cpp/src/parquet/column_writer.cc Outdated

Comment thread cpp/src/parquet/file_writer.cc Outdated

Comment thread cpp/src/parquet/file_writer.cc Outdated

github-actions Bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Oct 10, 2026

Copilot AI balanced review requested due to automatic review settings October 10, 2026 18:00

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 10, 2026

Copilot started reviewing on behalf of kszucs October 10, 2026 18:00 View session

github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 10, 2026

Copilot AI reviewed Oct 10, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 10, 2026 18:06

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 10, 2026

Copilot started reviewing on behalf of kszucs October 10, 2026 18:07 View session

kszucs added 5 commits October 10, 2026 20:07

[C++][Parquet] Keep the CDC chunking state across row groups

c70ed24

The column writer created the chunker for every column chunk, so the chunking state was reset at each row group. The file writer now keeps one chunker per leaf column and passes it to the column writers of all the row groups.

[C++][Parquet] Let the column writer compute its own level information

dcb5b84

The chunker stores the level information by value, so the file writer doesn't need to share it with the column writers. Computing it is cheap.

[C++][Parquet] Revert advancing the column ordinal before closing the…

81c63d0

… previous column The column ordinal and the column metadata fall out of step if closing the previous column writer throws and NextColumn() is called again, but that isn't specific to content defined chunking.

[C++][Parquet] Pass the content defined chunker through the public Co…

77b9299

…lumnWriter::Make() The column writer throws if content defined chunking is enabled without a chunker, so a separate private overload isn't needed.

kszucs force-pushed the cdc-row-group-reset branch from f20c5f0 to 77b9299 Compare October 10, 2026 18:09

github-actions Bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Oct 10, 2026

Copilot AI reviewed Oct 10, 2026

View reviewed changes

Copilot AI balanced review requested due to automatic review settings October 10, 2026 18:09

[C++][Parquet] Construct the content defined chunkers in place

4b44d4a

Drop ContentDefinedChunker::Make() and the move assignment operator, the file writer only needs the move constructor to keep the chunkers in a vector.

Copilot started reviewing on behalf of kszucs October 10, 2026 18:10 View session

Copilot AI reviewed Oct 10, 2026

View reviewed changes

Navigation Menu

Repository navigation

GH-51684: [C++][Parquet] Keep the CDC chunking state across row groups - #51685

GH-51684: [C++][Parquet] Keep the CDC chunking state across row groups#51685
kszucs wants to merge 6 commits into
apache:mainfrom
kszucs:cdc-row-group-reset

kszucs commented Oct 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

This comment was marked as duplicate.

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

kszucs commented Oct 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kszucs commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Deduplication results

Was AI used for this PR?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

This comment was marked as duplicate.

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

kszucs commented Oct 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kszucs commented Oct 2, 2026 •

edited

Loading