GH-3710: Tolerate unrecognized logical/physical type combinations when reading - #3711
GH-3710: Tolerate unrecognized logical/physical type combinations when reading#3711divjotarora wants to merge 3 commits into
Conversation
57ea343 to
8469645
Compare
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
I think most important question is how we make this transition and an end-to-end test.
divjotarora
left a comment
divjotarora
left a comment
There was a problem hiding this comment.
@emkornfield I added an e2e test that reads a golden file with an INT32 column annotated with UUID. We can add this file to parquet-testing as part of this work as well.
There was a problem hiding this comment.
is this just a place holder for the until the parquet testing file is merged?
There was a problem hiding this comment.
Yes, I'll update the PR once that one is merged
| protected PrimitiveType build(String name) { | ||
| try { | ||
| return validateAndBuild(name); | ||
| } catch (IllegalStateException e) { |
There was a problem hiding this comment.
it is possible to make this more specific? Also there is a general spec question on whether we should error for clearly invalid types (e.g. decimal with a precision that is too high on ints). This is probably also a spec level question.
There was a problem hiding this comment.
Good point, I agree this should be scoped to only throwing on invalid combinations. Right now everything throws IllegalStateException so we can't distinguish at this level. I added a new UnsupportedLogicalTypeAnnotation exception to distinguish.
emkornfield
left a comment
emkornfield
left a comment
There was a problem hiding this comment.
main concern is on the breadth of the exception cast.
2 participants
Rationale for this change
See apache/parquet-format#607 for rationale.
What changes are included in this PR?
This PR modifies parquet-java to gracefully handle 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, several unit tests added.
Are there any user-facing changes?
No.
Closes #3710