Repository navigation
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft: Arrow-rs #11733 has merged. This still needs a Parquet release and dependency upgrade that provide
FileEncryptionProperties::column_key_metadata; the current stock Parquet 60.0.0 dependency does not include it.Which issue does this PR close?
No existing issue. The reproduction, root cause and regression coverage are described below.
Rationale for this change
Converting Parquet encryption properties to DataFusion configuration can move a key identifier to a different column when only some column keys have metadata. The key bytes retain their original column associations, so direct-key file reads may still succeed while metadata-driven key retrieval requests an incorrect identifier.
A mixed configuration can contain
firstwith IDfirst-idandsecondwith no ID. Depending on map iteration order, the converted configuration can instead putfirst-idonsecondand leavefirstwithout an ID.The existing
column_keys()metadata vector skips columns without metadata, whereas its names and keys vectors include every column. Using the same position to index these vectors therefore loses the name-to-metadata association.What changes are included in this PR?
The draft isolates the consumer change so it can be evaluated alongside the merged upstream API. It needs a dependency update once the API is released; the submitted diff contains no Cargo dependency override.
What is the testing strategy for this PR?
The regression cases cover mixed, all-present and all-absent column metadata, including explicitly empty metadata and footer metadata preservation. Independent property maps exercise unspecified map iteration order. Complete property equality checks the reverse conversion; actual file assertions verify the serialized column IDs rather than relying only on successful explicit-key reads.
Local validation uses Parquet 60.0.0 with only the proposed accessor added through a temporary Cargo patch. Results apply to that validation setup, not the current stock dependency:
With an isolated Parquet 60 source directory containing the accessor from the upstream PR, the focused regression command is:
datafusion-common: 664 unit tests, four new integration tests and 81 doctests pass; five existing doctests are ignored.Are there any user-facing changes?
Encryption property conversion preserves each column's optional key metadata. No configuration fields or existing public signatures change.