GH-50910: [Parquet] Tolerate unrecognized logical/physical type combinations when reading - #50909
GH-50910: [Parquet] Tolerate unrecognized logical/physical type combinations when reading#50909divjotarora wants to merge 2 commits into
Conversation
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
github-actions
Bot
added
Component: Parquet
Component: C++
awaiting review
divjotarora
changed the title
|
|
4dc0ee3 to
6017b0e
Compare
There was a problem hiding this comment.
Changes in this file are due to pinning to a parquet-testing commit that contains the new file from apache/parquet-testing#122. The only file we actually use from this set is data/int32_with_uuid_logical_type.parquet.
6017b0e to
7062580
Compare
Reranko05
left a comment
Reranko05
left a comment
There was a problem hiding this comment.
Could you also add Component of the PR in the title, it will be easier for maintainers to review it.
GH-<Issue Number>: [<Component>] <Title>
divjotarora
changed the title
divjotarora
changed the title
github-actions
Bot
added
awaiting committer review
emkornfield
changed the title
| // annotation. | ||
| if (logical_type && !logical_type->is_nested() && | ||
| !logical_type->is_applicable(physical_type, element->type_length)) { | ||
| logical_type = UndefinedLogicalType::Make(); |
There was a problem hiding this comment.
The java implementation seems to log in this branch any reason you aren't doing the same? it is a larger change but at some point we might want to make this configurable?
There was a problem hiding this comment.
Added a log here as well.
it is a larger change but at some point we might want to make this configurable?
I'm not sure we need this to be configurable. It's not in parquet-java and likely would not be in arrow-rs based on previous changes made there. Is it standard to add such flags in this implementation?
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
a couple of questions, but seems reasonable.
3 participants
Rationale for this change
See apache/parquet-format#607 for rationale.
What changes are included in this PR?
This PR gracefully handles unrecognized logical/physical type combinations by dropping the logical type during the read and dropping any associated statistics for the relevant columns. Note that unrecognized logical types are already handled gracefully and no changes were required.
Are these changes tested?
Yes, via unit tests and an e2e test that reads a real file that contains an invalid type combination.
Are there any user-facing changes?
No