Repository navigation
fix(parquet): support non-primitive map keys in record reader - #11229
Merged
Merged
Conversation
Contributor
|
doc failure should be fixed by #11305 |
Contributor
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.
Which issue does this PR close?
Rationale for this change
The record API (
RowIter/TreeBuilder) asserted that map keys are primitive, so reading a file with e.g. struct- or list-keyed maps panicked. The Parquet spec has no such restriction and the arrow reader already handles these files.This picks up the fix from #7769 by @njaremko, which went stale waiting on a test file in apache/parquet-testing#87. Thanks @njaremko for the original fix.
What changes are included in this PR?
TreeBuilder::reader_tree. The key reader was already built viareader_treein the same way as the value reader, so no other reader changes are needed.Mapstores entries asVec<(Field, Field)>, so complex keys need noHash/Eq;Displayandto_json_valuealready handle non-primitive keys.Are these changes tested?
Yes.
test_compound_map_keywrites a file in memory withArrowWritercontaining a map with struct keys and a map with list keys (including an empty map and an empty-list key), then reads it back with the record API and checks the rows. Generating the file in the test avoids depending on apache/parquet-testing#87 and addresses the review feedback on #7769 (descriptive test name, only the relevant columns). The test panics without the fix.Are there any user-facing changes?
Files with non-primitive map keys can now be read with the record API instead of panicking. No API changes.
AI usage: this change was prepared with the help of AI assistants (Claude, OpenAI models). I reviewed the change and the reader's key/value level handling, and verified the test fails before and passes after the fix.