Remove primitive map key assertion on record reader#7769
Remove primitive map key assertion on record reader#7769njaremko wants to merge 2 commits intoapache:mainfrom
Conversation
njaremko
changed the title
njaremko
force-pushed
the
nathan_06-24-remove_primitive_map_key_assertion_on_record_reader
branch
from
2d8b0b9 to
ea6d5f6
Compare
njaremko
force-pushed
the
nathan_06-24-remove_primitive_map_key_assertion_on_record_reader
branch
from
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 |
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.