Repository navigation
Conversation
njaremko
changed the title
2d8b0b9 to
ea6d5f6
Compare
ea6d5f6 to
edd9b70
Compare
| } | ||
|
|
||
| #[test] | ||
| fn test_file_reader_rows_nullable1() { |
There was a problem hiding this comment.
A more descriptive name here would be nice...test_compound_map_key perhaps?
There was a problem hiding this comment.
lol, whoops, yeah, definitely need to fix this
|
|
||
| #[test] | ||
| fn test_file_reader_rows_nullable1() { | ||
| let rows = test_file_reader_rows("databricks.parquet", None).unwrap(); |
There was a problem hiding this comment.
Likewise, something more descriptive than "databricks".
There was a problem hiding this comment.
It was named this because it's a file generated by databricks with every supported column type, I'll look into regenerating it
| ] | ||
| ), | ||
| ( | ||
| "map_nested".to_string(), |
There was a problem hiding this comment.
This is the meat of the PR; would it be possible to only have this column in the test file? This is a lot to slog through and obscures the point of the test.
|
I'll update this PR with the requested changes once this gets merged: apache/parquet-testing#87 |
…_assertion_on_record_reader
THanks again @njaremko Given that we don't really control the timeline of the upstream parquet-testing repo, if you want to unblock this PR you could potentially copy the test file into the arrow-rs repo with a TODO comment that says to change to use the copy upstream in parquet-testing when apache/parquet-testing#87 is merged |
|
Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look |
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
…#11229) # Which issue does this PR close? - Closes apache#7768. # 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 apache#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? - Remove the primitive-key assertion in `TreeBuilder::reader_tree`. The key reader was already built via `reader_tree` in the same way as the value reader, so no other reader changes are needed. - `Map` stores entries as `Vec<(Field, Field)>`, so complex keys need no `Hash`/`Eq`; `Display` and `to_json_value` already handle non-primitive keys. # Are these changes tested? Yes. `test_compound_map_key` writes a file in memory with `ArrowWriter` containing 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 apache#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. Co-authored-by: Ed Seidl <etseidl@users.noreply.github.com> Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
3 participants
Note:
This PR has a test that requires a file that needs to be upstreamed
Which issue does this PR close?
Rationale for this change
There's no such requirement in the parquet logical type specification for map types. Spark used to have a similar assertion, that they've since removed
What changes are included in this PR?
Getting rid of the assertion, and adding a test
Are there any user-facing changes?
No
If there are any breaking changes to public APIs, please call them out.