Parquet: Move to ValueReader generation to a visitor #9063
Conversation
Fokko
changed the title
| } | ||
|
|
||
| @Override | ||
| public Optional<ParquetValueReader<?>> visit( |
There was a problem hiding this comment.
is this case UTF8: return new ParquetValueReaders.StringReader(desc); being covered here ?
There was a problem hiding this comment.
Yes, the old UTF8 OriginalType, maps to a StringType: https://github.com/apache/parquet-mr/blob/65bc51846010360f3dd4304103ec3c637776d7c9/parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java#L197-L198
|
@aokolnychyi do you have a spare cycle for reviewing this? |
There was a problem hiding this comment.
I think there's just a bug in the visit(timestampLogicalType) case, but everything else looks great to me. This does lead me to wonder though, do the parquet reader tests test all these data types?
I'll double check that. We don't need to need to couple any of those in this PR though.
| return tsMicrosType.shouldAdjustToUTC() | ||
| ? Optional.of(new TimestamptzReader(desc)) | ||
| : Optional.of(new TimestampReader(desc)); | ||
| } else if (timestampLogicalType.getUnit() == LogicalTypeAnnotation.TimeUnit.MICROS) { |
There was a problem hiding this comment.
I think this should be == LogicalTypeAnnotation.TimeUnit.MILLIS? Not Micros
There was a problem hiding this comment.
Oof, that's a good one! I would this part to be thoroughly tested as well.
|
Thanks @jacobmarble, @nk1506 & @amogh-jahagirdar for the review 🙌 |
* Parquet: Move to the visitor * Add one more edge case * Thanks Amogh!
* Parquet: Move to the visitor * Add one more edge case * Thanks Amogh!
* Parquet: Move to the visitor * Add one more edge case * Thanks Amogh!
This will replace the switch statement that relies on the deprecated
getOriginalType()with a visitor pattern of the newLogicalTypeAnnotation. OriginalType does not include support for Nanosecond precision timestamps.