GH-3710: Tolerate unrecognized logical/physical type combinations when reading - #3711
divjotarora wants to merge 7 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.
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.
0e17db9 to
8a64bd5
Compare
|
Thanks for the suggestions @RussellSpitzer, I've applied the new code structure now |
RussellSpitzer
left a comment
RussellSpitzer
left a comment
There was a problem hiding this comment.
Looks good to me now! I left one note on the missing test case.
I'm on board with setup of
We tolerate wrong logical type on physical type except for those few cases we have already explicitly noted are bad (Decimal with the wrong precision)
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Left one nit, but this looks good to me 👍
| .named("root"))) | ||
| .isInstanceOf(IllegalStateException.class) | ||
| .hasMessage("TIMESTAMP(MICROS,false) can only annotate INT64"); | ||
| .hasMessage("TIMESTAMP(MICROS,false) can only annotate [INT64]"); |
There was a problem hiding this comment.
nit: Maybe we can drop the brackets to avoid having to update these tests
There was a problem hiding this comment.
This was an intentional change as there might be multiple valid physical types and I felt printing as an array helps readability
4 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