-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Remove primitive map key assertion on record reader - #7769
Conversation
2d8b0b9 to
ea6d5f6
Compare
ea6d5f6 to
edd9b70
Compare
@etseidl
etseidl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for correcting this oversight @njaremko. FWIW the arrow API handles the test file as expected. Just a few nits with the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more descriptive name here would be nice...test_compound_map_key perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol, whoops, yeah, definitely need to fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, something more descriptive than "databricks".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was named this because it's a file generated by databricks with every supported column type, I'll look into regenerating it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
njaremko
commented
Jun 27, 2025
I'll update this PR with the requested changes once this gets merged: apache/parquet-testing#87
..._assertion_on_record_reader
alamb
commented
Jul 13, 2025
This PR has a test that requires a file that needs to be upstreamed
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
alamb
commented
Sep 8, 2025
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.
Uh oh!
There was an error while loading. Please reload this page.
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.