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 6 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 |
| assertThat(pt.getLogicalTypeAnnotation()).isNull(); | ||
| assertThat(pt.columnOrder().getColumnOrderName()).isEqualTo(ColumnOrder.ColumnOrderName.UNDEFINED); | ||
| } | ||
| } |
There was a problem hiding this comment.
Since we are still rejecting Decimals with bad precessions with a hard error (I think this is a good choice), we need a test to make sure that still happens when "ignoreUnsupportedLogicalAnnotations()" is used.
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)
3 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