From bdc2ec5e9dfe5d6dc5e6babc9c5038f7662d9bdd Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Wed, 16 Sep 2026 15:46:16 +0200 Subject: [PATCH 1/6] Fix column index mismatch in FileReaderImpl::DecodeRowGroups 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 --- cpp/src/parquet/arrow/reader.cc | 13 ++++++++++--- .../pyarrow/tests/parquet/test_encryption.py | 18 +++++++++--------- 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/cpp/src/parquet/arrow/reader.cc b/cpp/src/parquet/arrow/reader.cc index eca83e8576da..64ae41a25d46 100644 --- a/cpp/src/parquet/arrow/reader.cc +++ b/cpp/src/parquet/arrow/reader.cc @@ -1379,11 +1379,18 @@ Future> FileReaderImpl::DecodeRowGroups( // OptionalParallelForAsync requires an executor if (!cpu_executor) cpu_executor = ::arrow::internal::GetCpuThreadPool(); - auto read_column = [row_groups, self, this](size_t i, - std::shared_ptr reader) + // `readers` only holds the requested columns, so its index `i` is a position within + // the selection, not the column's actual index in the row group. Map back to the + // real column index (as GetFieldReaders does internally) before calling ReadColumn, + // which indexes RowGroupMetaData::ColumnChunk() by the latter. + ARROW_ASSIGN_OR_RAISE(std::vector field_indices, + manifest_.GetFieldIndices(column_indices)); + + auto read_column = [row_groups, field_indices, self, this]( + size_t i, std::shared_ptr reader) -> ::arrow::Result> { std::shared_ptr<::arrow::ChunkedArray> column; - RETURN_NOT_OK(ReadColumn(static_cast(i), row_groups, reader.get(), &column)); + RETURN_NOT_OK(ReadColumn(field_indices[i], row_groups, reader.get(), &column)); return column; }; auto make_table = [result_schema, row_groups, self, diff --git a/python/pyarrow/tests/parquet/test_encryption.py b/python/pyarrow/tests/parquet/test_encryption.py index 6a3842f3edf8..010ffef54181 100644 --- a/python/pyarrow/tests/parquet/test_encryption.py +++ b/python/pyarrow/tests/parquet/test_encryption.py @@ -501,13 +501,12 @@ def validate_kms_connection_config(kms_connection_config): validate_kms_connection_config(kms_connection_config_1) -@pytest.mark.xfail(reason="Plaintext footer - reading plaintext column subset" - " reads encrypted columns too") def test_encrypted_parquet_write_read_plain_footer_single_wrapping( tempdir, data_table): - """Write an encrypted parquet, with plaintext footer - and with single wrapping, - verify it's encrypted, and then read plaintext columns.""" + """ + Write an encrypted parquet, with plaintext footer and with single wrapping, + verify it's encrypted, and then read plaintext columns. + """ path = tempdir / PARQUET_NAME # Encrypt the footer with the footer key, @@ -536,10 +535,11 @@ def kms_factory(kms_connection_configuration): write_encrypted_parquet(path, data_table, encryption_config, kms_connection_config, crypto_factory) - # # Read without decryption properties only the plaintext column - # result = pq.ParquetFile(path) - # result_table = result.read(columns='c', use_threads=False) - # assert table.num_rows == result_table.num_rows + # Read without decryption properties only the plaintext column + result = pq.ParquetFile(path) + result_table = result.read(columns='c', use_threads=False) + assert data_table.num_rows == result_table.num_rows + assert data_table.select(['c']).equals(result_table) def test_encrypted_parquet_write_read_external(tempdir, data_table, From b7aa718097350154e08bab217bea29dc0e5d998e Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Wed, 16 Sep 2026 19:28:00 +0000 Subject: [PATCH 2/6] Fix ReadColumn's records-to-read to use row group row count directly 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 --- cpp/src/parquet/arrow/reader.cc | 10 +-- .../pyarrow/tests/parquet/test_encryption.py | 69 +++++++++++-------- 2 files changed, 46 insertions(+), 33 deletions(-) diff --git a/cpp/src/parquet/arrow/reader.cc b/cpp/src/parquet/arrow/reader.cc index 64ae41a25d46..88ce7c1c3f30 100644 --- a/cpp/src/parquet/arrow/reader.cc +++ b/cpp/src/parquet/arrow/reader.cc @@ -271,13 +271,13 @@ class FileReaderImpl : public FileReader { Status ReadColumn(int i, const std::vector& row_groups, ColumnReader* reader, std::shared_ptr* out) { BEGIN_PARQUET_CATCH_EXCEPTIONS - // TODO(wesm): This calculation doesn't make much sense when we have repeated - // schema nodes + // NextBatch()'s size is a number of records (rows), not leaf values, so use the + // 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). int64_t records_to_read = 0; for (auto row_group : row_groups) { - // 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(); } #ifdef ARROW_WITH_OPENTELEMETRY std::string column_name = reader_->metadata()->schema()->Column(i)->name(); diff --git a/python/pyarrow/tests/parquet/test_encryption.py b/python/pyarrow/tests/parquet/test_encryption.py index 010ffef54181..5439c3bf0e38 100644 --- a/python/pyarrow/tests/parquet/test_encryption.py +++ b/python/pyarrow/tests/parquet/test_encryption.py @@ -505,41 +505,54 @@ def test_encrypted_parquet_write_read_plain_footer_single_wrapping( tempdir, data_table): """ Write an encrypted parquet, with plaintext footer and with single wrapping, - verify it's encrypted, and then read plaintext columns. + verify it's encrypted, and then read plaintext columns. Runs once with a + flat schema and once where the encrypted column `b` is itself a nested + (struct) field. """ path = tempdir / PARQUET_NAME - # Encrypt the footer with the footer key, - # encrypt column `a` and column `b` with another key, - # keep `c` plaintext - encryption_config = pe.EncryptionConfiguration( - footer_key=FOOTER_KEY_NAME, - column_keys={ - COL_KEY_NAME: ["a", "b"], - }, - plaintext_footer=True, - double_wrapping=False) + for nested in [False, True]: + if nested: + table = pa.Table.from_pydict({ + 'a': pa.array([1, 2, 3]), + 'b': pa.array( + [{'x': 1, 'y': 2}, {'x': 3, 'y': 4}, {'x': 5, 'y': 6}], + type=pa.struct([('x', pa.int32()), ('y', pa.int32())])), + 'c': pa.array(['x', 'y', 'z']) + }) + else: + table = data_table + + # Encrypt the footer with the footer key, + # encrypt column `a` and column `b` with another key, keep `c` plaintext + encryption_config = pe.EncryptionConfiguration( + footer_key=FOOTER_KEY_NAME, + column_keys={ + COL_KEY_NAME: ["a", "b"], + }, + plaintext_footer=True, + double_wrapping=False) - kms_connection_config = pe.KmsConnectionConfig( - custom_kms_conf={ - FOOTER_KEY_NAME: FOOTER_KEY.decode("UTF-8"), - COL_KEY_NAME: COL_KEY.decode("UTF-8"), - } - ) + kms_connection_config = pe.KmsConnectionConfig( + custom_kms_conf={ + FOOTER_KEY_NAME: FOOTER_KEY.decode("UTF-8"), + COL_KEY_NAME: COL_KEY.decode("UTF-8"), + } + ) - def kms_factory(kms_connection_configuration): - return InMemoryKmsClient(kms_connection_configuration) + def kms_factory(kms_connection_configuration): + return InMemoryKmsClient(kms_connection_configuration) - crypto_factory = pe.CryptoFactory(kms_factory) - # Write with encryption properties - write_encrypted_parquet(path, data_table, encryption_config, - kms_connection_config, crypto_factory) + crypto_factory = pe.CryptoFactory(kms_factory) + # Write with encryption properties + write_encrypted_parquet(path, table, encryption_config, + kms_connection_config, crypto_factory) - # Read without decryption properties only the plaintext column - result = pq.ParquetFile(path) - result_table = result.read(columns='c', use_threads=False) - assert data_table.num_rows == result_table.num_rows - assert data_table.select(['c']).equals(result_table) + # Read the plaintext column without decryption properties + result = pq.ParquetFile(path) + result_table = result.read(columns='c', use_threads=False) + assert table.num_rows == result_table.num_rows + assert table.select(['c']).equals(result_table) def test_encrypted_parquet_write_read_external(tempdir, data_table, From f2b145330c41b49bc3d3d538f0edc093f62821d2 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Thu, 17 Sep 2026 07:42:58 +0000 Subject: [PATCH 3/6] Fix test_dataset_encryption.py expectations for the DecodeRowGroups fix 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 --- python/pyarrow/tests/test_dataset_encryption.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/python/pyarrow/tests/test_dataset_encryption.py b/python/pyarrow/tests/test_dataset_encryption.py index 0ef3931a4cf6..ceb75ca603b7 100644 --- a/python/pyarrow/tests/test_dataset_encryption.py +++ b/python/pyarrow/tests/test_dataset_encryption.py @@ -115,11 +115,9 @@ def do_test_dataset_encryption_decryption(table, extra_column_path=None): if extra_column_path: keys = dict(**KEYS, **{EXTRA_COL_KEY_NAME: EXTRA_COL_KEY}) column_keys = dict(**COLUMN_KEYS, **{EXTRA_COL_KEY_NAME: [extra_column_path]}) - extra_column_name = extra_column_path.split(".")[0] else: keys = KEYS column_keys = COLUMN_KEYS - extra_column_name = None # define the actual test def assert_decrypts( @@ -235,13 +233,10 @@ def assert_decrypts( for key_name, key in keys.items() if key_name in [FOOTER_KEY_NAME, column_key_name]} - # that one encrypted column can only be read - # if it is not a column path / nested field - plaintext_and_one_success = encrypted_column_name != extra_column_name plaintext_and_one = plaintext_column_names + [encrypted_column_name] assert_decrypts(read_keys, plaintext_column_names, True) - assert_decrypts(read_keys, plaintext_and_one, plaintext_and_one_success) + assert_decrypts(read_keys, plaintext_and_one, True) assert_decrypts(read_keys, encrypted_column_names, False) assert_decrypts(read_keys, all_column_names, False) From fa0af90617078450c99e0db8fe6918e93ab93527 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Thu, 17 Sep 2026 09:57:03 +0200 Subject: [PATCH 4/6] Tighten focus of comments --- cpp/src/parquet/arrow/reader.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cpp/src/parquet/arrow/reader.cc b/cpp/src/parquet/arrow/reader.cc index 88ce7c1c3f30..8fd9efc0a93f 100644 --- a/cpp/src/parquet/arrow/reader.cc +++ b/cpp/src/parquet/arrow/reader.cc @@ -272,9 +272,7 @@ class FileReaderImpl : public FileReader { std::shared_ptr* out) { BEGIN_PARQUET_CATCH_EXCEPTIONS // NextBatch()'s size is a number of records (rows), not leaf values, so use the - // 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 own row count directly rather than some column's num_values(). int64_t records_to_read = 0; for (auto row_group : row_groups) { records_to_read += reader_->metadata()->RowGroup(row_group)->num_rows(); From 322c75586d3d0f6632e52e0fdd2a3724bc072627 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Thu, 17 Sep 2026 10:04:09 +0200 Subject: [PATCH 5/6] Close parquet file after reading --- python/pyarrow/tests/parquet/test_encryption.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/python/pyarrow/tests/parquet/test_encryption.py b/python/pyarrow/tests/parquet/test_encryption.py index 5439c3bf0e38..5a34f32bc3fd 100644 --- a/python/pyarrow/tests/parquet/test_encryption.py +++ b/python/pyarrow/tests/parquet/test_encryption.py @@ -549,10 +549,10 @@ def kms_factory(kms_connection_configuration): kms_connection_config, crypto_factory) # Read the plaintext column without decryption properties - result = pq.ParquetFile(path) - result_table = result.read(columns='c', use_threads=False) - assert table.num_rows == result_table.num_rows - assert table.select(['c']).equals(result_table) + with pq.ParquetFile(path) as result: + result_table = result.read(columns='c', use_threads=False) + assert table.num_rows == result_table.num_rows + assert table.select(['c']).equals(result_table) def test_encrypted_parquet_write_read_external(tempdir, data_table, From 681220dc60e0d886f2d12c0d833e38f0567f6650 Mon Sep 17 00:00:00 2001 From: Enrico Minack Date: Thu, 17 Sep 2026 10:45:37 +0200 Subject: [PATCH 6/6] Revert changes in FileReaderImpl::DecodeRowGroups --- cpp/src/parquet/arrow/reader.cc | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/cpp/src/parquet/arrow/reader.cc b/cpp/src/parquet/arrow/reader.cc index 8fd9efc0a93f..bb8bacc7751a 100644 --- a/cpp/src/parquet/arrow/reader.cc +++ b/cpp/src/parquet/arrow/reader.cc @@ -1377,18 +1377,11 @@ Future> FileReaderImpl::DecodeRowGroups( // OptionalParallelForAsync requires an executor if (!cpu_executor) cpu_executor = ::arrow::internal::GetCpuThreadPool(); - // `readers` only holds the requested columns, so its index `i` is a position within - // the selection, not the column's actual index in the row group. Map back to the - // real column index (as GetFieldReaders does internally) before calling ReadColumn, - // which indexes RowGroupMetaData::ColumnChunk() by the latter. - ARROW_ASSIGN_OR_RAISE(std::vector field_indices, - manifest_.GetFieldIndices(column_indices)); - - auto read_column = [row_groups, field_indices, self, this]( - size_t i, std::shared_ptr reader) + auto read_column = [row_groups, self, this](size_t i, + std::shared_ptr reader) -> ::arrow::Result> { std::shared_ptr<::arrow::ChunkedArray> column; - RETURN_NOT_OK(ReadColumn(field_indices[i], row_groups, reader.get(), &column)); + RETURN_NOT_OK(ReadColumn(static_cast(i), row_groups, reader.get(), &column)); return column; }; auto make_table = [result_schema, row_groups, self,