GH-51361: [C++][Parquet] Fix column index mismatch in FileReaderImpl::DecodeRowGroups by EnricoMi · Pull Request #51362 · apache/arrow

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

Rationale for this change

What changes are included in this PR?

Use the right indices in FileReaderImpl::DecodeRowGroups when calling ReadColumn. Reading a subset of the Parquet file's columns would use the index among those selected columns, rather than the Parquet file's column index.

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.

+  ARROW_ASSIGN_OR_RAISE(std::vector<int> field_indices,
+                        manifest_.GetFieldIndices(column_indices));


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>

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

+  // real column index (as GetFieldReaders does internally) before calling ReadColumn,
+  // which indexes RowGroupMetaData::ColumnChunk() by the latter.
+  ARROW_ASSIGN_OR_RAISE(std::vector<int> field_indices,
+                        manifest_.GetFieldIndices(column_indices));


adamreeve

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

adamreeve · 2026-09-16T22:07:38Z

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


NextBatch()'s size is a number of records (rows), not leaf values

I think this is correct after looking through the code, but this is not very clear. Maybe NextBatch and LoadBatch methods could have better documentation comments. The number of records ends up being passed through to RecordReader::ReadRecords which is better documented.

adamreeve · 2026-09-16T22:10:54Z

+    // row group's own row count directly rather than some column's num_values() (which
+    // (a) requires picking a column, a prior source of index-confusion bugs, and
+    // (b) over-counts for repeated schema nodes, counting elements rather than rows).


Suggested change

// row group's own row count directly rather than some column's num_values() (which

// (a) requires picking a column, a prior source of index-confusion bugs, and

// (b) over-counts for repeated schema nodes, counting elements rather than rows).

// row group's row count directly.

It's pretty common for AI to generate comments like this that compare the new code to the old way it was done, but I don't think these are helpful to leave in the code. The first part of the comment is enough to explain why num_rows is used, and any explanation of the need for the change can go in the PR description.

adamreeve · 2026-09-16T22:24:14Z

      -> ::arrow::Result<std::shared_ptr<::arrow::ChunkedArray>> {
    std::shared_ptr<::arrow::ChunkedArray> column;
-    RETURN_NOT_OK(ReadColumn(static_cast<int>(i), row_groups, reader.get(), &column));
+    RETURN_NOT_OK(ReadColumn(field_indices[i], row_groups, reader.get(), &column));


It looks like this should be using column_indices to get the Parquet leaf column index.

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

Comment on lines +1386 to +1387

ARROW_ASSIGN_OR_RAISE(std::vector<int> field_indices,

manifest_.GetFieldIndices(column_indices));

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

adamreeve reviewed Sep 16, 2026

View reviewed changes

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

Navigation Menu

Uh oh!

GH-51361: [C++][Parquet] Fix column index mismatch in FileReaderImpl::DecodeRowGroups - #51362

GH-51361: [C++][Parquet] Fix column index mismatch in FileReaderImpl::DecodeRowGroups#51362
EnricoMi wants to merge 2 commits into
apache:mainfrom
EnricoMi:fix-decode-row-groups-column-index

EnricoMi commented Sep 16, 2026 •

edited by github-actions Bot

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!

Copilot AI left a comment

Uh oh!

adamreeve left a comment

Uh oh!

adamreeve Sep 16, 2026

Uh oh!

adamreeve Sep 16, 2026

Uh oh!

adamreeve Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		ARROW_ASSIGN_OR_RAISE(std::vector<int> field_indices,
		manifest_.GetFieldIndices(column_indices));

Uh oh!

Conversation

EnricoMi commented Sep 16, 2026 • edited by github-actions Bot 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!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

adamreeve left a comment

Choose a reason for hiding this comment

Uh oh!

adamreeve Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeve Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

adamreeve Sep 16, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EnricoMi commented Sep 16, 2026 •

edited by github-actions Bot

Loading