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 2 commits into
Conversation
57ea343 to
8469645
Compare
| return validateAndBuild(name); | ||
| } catch (IllegalStateException e) { | ||
| boolean canDrop = dropUnsupportedLogicalTypeCombinations | ||
| && !(logicalTypeAnnotation instanceof LogicalTypeAnnotation.DecimalLogicalTypeAnnotation); |
There was a problem hiding this comment.
why is this special cased?
There was a problem hiding this comment.
This is leftover from an old version of the implementation, removed the special case
| assertThat(roundTrip.getNanCounts()).containsExactly(1L, 0L, 0L); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
not sure where the right file for it is but it would be good to have a more end-to-end test.
There was a problem hiding this comment.
Added a new TestReadInvalidTypeCombination
| * | ||
| * @return this builder for method chaining | ||
| */ | ||
| public THIS dropUnsupportedLogicalTypeCombinations() { |
There was a problem hiding this comment.
how is this method expected to be called in practice?
There was a problem hiding this comment.
This is called directly in ParquetMetadataConverter.java here
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.
| * | ||
| * @return this builder for method chaining | ||
| */ | ||
| public THIS dropUnsupportedLogicalTypeCombinations() { |
There was a problem hiding this comment.
This is called directly in ParquetMetadataConverter.java here
| return validateAndBuild(name); | ||
| } catch (IllegalStateException e) { | ||
| boolean canDrop = dropUnsupportedLogicalTypeCombinations | ||
| && !(logicalTypeAnnotation instanceof LogicalTypeAnnotation.DecimalLogicalTypeAnnotation); |
There was a problem hiding this comment.
This is leftover from an old version of the implementation, removed the special case
| assertThat(roundTrip.getNanCounts()).containsExactly(1L, 0L, 0L); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Added a new TestReadInvalidTypeCombination
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