RussellSpitzer commented on code in PR #17196:
URL: https://github.com/apache/iceberg/pull/17196#discussion_r3591006194
##########
core/src/main/java/org/apache/iceberg/avro/SchemaToType.java:
##########
@@ -193,19 +196,30 @@ public Type logicalType(Schema primitive, LogicalType
logical) {
} else if (logical instanceof LogicalTypes.TimestampMillis
|| logical instanceof LogicalTypes.TimestampMicros) {
- if (AvroSchemaUtil.isTimestamptz(primitive)) {
+ if (AvroSchemaUtil.isTimestamptz(primitive, legacyTimestampMapping)) {
return Types.TimestampType.withZone();
} else {
return Types.TimestampType.withoutZone();
}
} else if (logical instanceof LogicalTypes.TimestampNanos) {
- if (AvroSchemaUtil.isTimestamptz(primitive)) {
+ if (AvroSchemaUtil.isTimestamptz(primitive, legacyTimestampMapping)) {
return Types.TimestampNanoType.withZone();
} else {
return Types.TimestampNanoType.withoutZone();
}
+ } else if (logical instanceof LogicalTypes.LocalTimestampMillis
Review Comment:
I'm not sure I understand. How would we produce local-timestamp-* after they
upgrade? It would only be produced by writers with the legacy flag off which
would not be the default?
This is guarding against users who were manually creating local-timestamp in
their Avro schemas (which I think has to come from outside our library) but are
relying on this function to return a "long" in that use-case.
There is an issue with "timestamp-micros" (not local) but that is handled
correctly by passing through the legacy mode
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
