GH-51361: [C++][Parquet] Derive records_to_read in FileReaderImpl::ReadColumn from RowGroup by EnricoMi · Pull Request #51362 · apache/arrow

EnricoMi · 2026-09-16T19:05:11Z

Rationale for this change

What changes are included in this PR?

Method FileReaderImpl::ReadColumn should derive records_to_read from the RowGroup rather than ColumnChunk's num_values. Deriving the right ColumnChunk index in FileReaderImpl::DecodeRowGroups is not trivial for nested schemas. This simplifies FileReaderImpl::ReadColumn and fixes #51361.

This was silently masked for full-schema reads and for columns with identical num_values(), but surfaces as a hard failure when an earlier, unselected column requires decryption: reading only a trailing plaintext column of a partially column-key-encrypted, plaintext-footer Parquet file threw "Cannot decrypt ColumnMetadata" even though the requested column was never encrypted.

This never corrupts data on unencrypted files: ReadColumn's wrong index is only ever used to look up ColumnChunk(i)->num_values(), a count fed into the already-correct reader as an upper bound on how many records to decode. Every row contributes at least one definition/repetition-level entry, so num_values() for any column is always >= that row group's true row count, and every column in a row group shares the same row count.

Are these changes tested?

Yes, in the context of reading a plaintext column of a partially encrypted Parquet file. This cannot be tested with non-encrypted files.

Are there any user-facing changes?

No.

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.

PR code and description written by:

Human
AI

Reviewed before submission by:

Human
AI
Not reviewed

GitHub Issue: [C++][Parquet] Partially encrypted parquet files with plaintext footers should allow reading plaintext columns #51361

The read_column callback used its position within the filtered `readers` vector as the column's real Parquet index when calling ReadColumn(), which looks up RowGroupMetaData::ColumnChunk() by absolute column index. Selecting a non-prefix column subset (e.g. column 2 out of 3) therefore read metadata for the wrong column. This was silently masked for full-schema reads and for columns with identical num_values(), but surfaces as a hard failure when an earlier, unselected column requires decryption: reading only a trailing plaintext column of a partially column-key-encrypted, plaintext-footer Parquet file threw "Cannot decrypt ColumnMetadata" even though the requested column was never encrypted. This never corrupts data on unencrypted files: ReadColumn's wrong index is only ever used to look up ColumnChunk(i)->num_values(), a count fed into the *already-correct* reader as an upper bound on how many records to decode. Every row contributes at least one definition/repetition-level entry, so num_values() for any column is always >= that row group's true row count, and every column in a row group shares the same row count. Map the selection position back to the real column index via manifest_.GetFieldIndices(), the same lookup GetFieldReaders() already performs internally. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

github-actions · 2026-09-16T19:05:34Z

Thanks for opening a pull request!

This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format.

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

After updating the title, you can mark the pull request as ready for review.

🟡 Changes recommended

The column-index mapping can still read or decrypt the wrong column for nested schemas.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR fixes column-index handling for partial Parquet reads involving encrypted files with plaintext footers.

Changes:

Updates row-group column decoding index handling.
Enables the plaintext-column encryption regression test.
A critical nested-schema index issue remains unresolved.

File summaries

File	Summary
`python/pyarrow/tests/parquet/test_encryption.py`	Enables regression coverage for reading an unencrypted column subset.
`cpp/src/parquet/arrow/reader.cc`	Adjusts column selection before decoding, but still passes incorrect indices for nested schemas.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

Resolve the moderate tracing/index-mapping issue in reader.cc.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

adamreeve

Nice find thanks Enrico. I think the copilot comment is correct and explains the test failures. I also have some minor suggestions.

This fixes FileReaderImpl::DecodeRowGroups for Parquet files with encrypted nested columns. Both the original flat-schema bug and the nested-schema one found by the struct-column regression test came from the same root cause: FileReaderImpl::ReadColumn() derived "how many records to decode" from some *other* column's ColumnChunk(i)->num_values(), keyed by an index that doesn't reliably identify the column actually being read (a selection position pre-fix, a top-level field position post-fix - neither matches the raw leaf-column index num_values() needs, once nesting is involved). NextBatch()'s size argument is a record (row) count, not a leaf value count, and RowGroupMetaData::num_rows() already gives that directly - no column index needed at all. Every row group has one row count shared by every column in it, so there was never a reason to route this through a specific column's metadata in the first place. This removes the index-confusion bug class entirely rather than chasing it into a third index space, and resolves the pre-existing "TODO(wesm): This calculation doesn't make much sense when we have repeated schema nodes" comment: num_values() over-counts for repeated fields (it counts elements, not rows), which is exactly what that TODO was flagging. Verified with a from-source build (cpp/build, PARQUET_REQUIRE_ENCRYPTION=ON): parquet-arrow-reader-writer-test passes all 824 runnable tests (with PARQUET_TEST_DATA set; the other 8 are pre-existing skips for legacy opt-in features). Verified through the real Python API too, via a from-source pyarrow install (pyarrow-dev venv, editable install against this repo's python/, linked against a freshly rebuilt libparquet.so): both test_encrypted_parquet_write_read_plain_footer_single_wrapping and its nested-schema sibling (test_encrypted_parquet_write_read_plain_footer_single_wrapping_nested_schema) now pass; the latter's xfail marker is removed since it reliably xpassed. The rest of python/pyarrow/tests/parquet/ shows no regressions (320 passed; remaining failures are pre-existing environment gaps - a missing tzdata package - unrelated to this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

do_test_dataset_encryption_decryption() expected reading a column selection of "plaintext columns + the one parametrized encrypted column" to raise ValueError("Unknown master key") whenever that column was the test's extra/parametrized one (list, map, struct, or a deep-nested path) rather than one of the standard column-key columns (n_legs, animal) - via `plaintext_and_one_success = encrypted_column_name != extra_column_name`, which (by construction) is always False for that column and always True for the standard ones. That expectation held only because of the DecodeRowGroups() column index bug: selecting a nested-field-backed column could spuriously touch a genuinely-unavailable-key column's metadata and throw for the wrong reason. With the read now correctly scoped to only the selected columns, and the parametrized column's own key being present in read_keys for that case, there is no missing key and the read legitimately succeeds - confirmed for every parametrization (list, list.list.element, map and its two leaf paths, struct and both its leaves, and all four col.* deep-nested paths). Verified: all 16 tests in test_dataset_encryption.py pass (15 passed, 1 skipped); python/pyarrow/tests/parquet/ unaffected (only the pre-existing, unrelated missing-tzdata failures remain). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Copilot

🟡 Changes recommended

Unresolved test resource-lifetime and column-index mapping issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

cpp/src/parquet/arrow/reader.cc:1393

GetFieldIndices returns top-level Arrow field positions, not physical Parquet leaf indices. For a schema such as a, struct b, c, selecting c yields field_indices[i] == 2 while the leaf index is 3, so this makes the OpenTelemetry span in ReadColumn report the wrong column (b.y instead of c) and does not provide the "real column index" described above. Preserve the original column_indices[i] for this argument, or remove this now-unnecessary mapping.

    RETURN_NOT_OK(ReadColumn(field_indices[i], row_groups, reader.get(), &column));

Files reviewed: 3/3 changed files
Comments generated: 1
Review effort level: Lite

EnricoMi · 2026-09-17T08:32:45Z

With

-      records_to_read +=
-          reader_->metadata()->RowGroup(row_group)->ColumnChunk(i)->num_values();
+      records_to_read += reader_->metadata()->RowGroup(row_group)->num_rows();

the encryption-related issue in method ReadColumn is fixed. The index i is then only used to report column information to OpenTelemetry, which used to be incorrect when subset of columns are read from the Parquet file.

I deem fixing that as out-of-scope of this PR. I have created issue #51370.

Copilot

🟢 Approval recommended

No unresolved blocking issues were identified.

Review details

Files reviewed: 3/3 changed files
Comments generated: 0 new
Review effort level: Lite

Copilot AI lite review requested due to automatic review settings September 16, 2026 19:05

EnricoMi requested review from AlenkaF, HuaHuaY, pitrou, raulcd, rok and wgtmac as code owners September 16, 2026 19:05

github-actions Bot marked this pull request as draft September 16, 2026 19:05

github-actions Bot added Component: Parquet Component: C++ Component: Python labels Sep 16, 2026

Copilot started reviewing on behalf of EnricoMi September 16, 2026 19:05 View session

github-actions Bot added the awaiting review Awaiting review label Sep 16, 2026

EnricoMi changed the title ~~Fix column index mismatch in FileReaderImpl::DecodeRowGroups~~ GH-51361: [C++][Parquet] Fix column index mismatch in FileReaderImpl::DecodeRowGroups Sep 16, 2026

EnricoMi marked this pull request as ready for review September 16, 2026 19:06

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread cpp/src/parquet/arrow/reader.cc Outdated

Copilot AI review requested due to automatic review settings September 16, 2026 19:44

Copilot started reviewing on behalf of EnricoMi September 16, 2026 19:45 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Comment thread cpp/src/parquet/arrow/reader.cc Outdated

adamreeve reviewed Sep 16, 2026

View reviewed changes

Comment thread cpp/src/parquet/arrow/reader.cc

Comment thread cpp/src/parquet/arrow/reader.cc Outdated

Comment thread cpp/src/parquet/arrow/reader.cc Outdated

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 16, 2026

Enrico Minack and others added 2 commits September 17, 2026 09:31

EnricoMi force-pushed the fix-decode-row-groups-column-index branch from 2d233a0 to f2b1453 Compare September 17, 2026 07:51

Copilot AI review requested due to automatic review settings September 17, 2026 07:51

Copilot started reviewing on behalf of EnricoMi September 17, 2026 07:51 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Comment thread python/pyarrow/tests/parquet/test_encryption.py Outdated

Enrico Minack added 2 commits September 17, 2026 09:57

Tighten focus of comments

fa0af90

Close parquet file after reading

322c755

EnricoMi mentioned this pull request Sep 17, 2026

[C++][Parquet] OpenTelemetry possibly reporting wrong column in ColumnReaderImpl::ReadColumn #51370

Closed

EnricoMi changed the title ~~GH-51361: [C++][Parquet] Fix column index mismatch in FileReaderImpl::DecodeRowGroups~~ GH-51361: [C++][Parquet] Derive records_to_read in FileReaderImpl::ReadColumn from RowGroup Sep 17, 2026

Revert changes in FileReaderImpl::DecodeRowGroups

681220d

Copilot AI review requested due to automatic review settings September 17, 2026 09:01

Copilot started reviewing on behalf of EnricoMi September 17, 2026 09:01 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

adamreeve approved these changes Sep 17, 2026

View reviewed changes

adamreeve merged commit 70f5c26 into apache:main Sep 20, 2026
57 of 58 checks passed

adamreeve removed the awaiting committer review Awaiting committer review label Sep 20, 2026

EnricoMi deleted the fix-decode-row-groups-column-index branch September 22, 2026 08:19

Navigation Menu

GH-51361: [C++][Parquet] Derive records_to_read in FileReaderImpl::ReadColumn from RowGroup - #51362

GH-51361: [C++][Parquet] Derive records_to_read in FileReaderImpl::ReadColumn from RowGroup #51362
adamreeve merged 6 commits into
apache:mainfrom
EnricoMi:fix-decode-row-groups-column-index

EnricoMi commented Sep 16, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 16, 2026

Uh oh!

github-actions Bot commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

adamreeve left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

EnricoMi commented Sep 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

EnricoMi commented Sep 16, 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?

Was AI used for this PR?

Uh oh!

github-actions Bot commented Sep 16, 2026

Uh oh!

github-actions Bot commented Sep 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

EnricoMi commented Sep 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EnricoMi commented Sep 16, 2026 •

edited

Loading