Uh oh!
There was an error while loading. Please reload this page.
Avro: Encode non-zone timestamps with local-timestamp logical types - #16577
Avro: Encode non-zone timestamps with local-timestamp logical types#16577Shekharrajak wants to merge 7 commits into
Conversation
| private static final Schema TIME_SCHEMA = | ||
| LogicalTypes.timeMicros().addToSchema(Schema.create(Schema.Type.LONG)); | ||
| private static final Schema TIMESTAMP_SCHEMA = | ||
| LogicalTypes.timestampMicros().addToSchema(Schema.create(Schema.Type.LONG)); |
There was a problem hiding this comment.
Switch writer to emit local-timestamp-micros for timestamp and local-timestamp-nanos for timestamp_ns,
| return Types.TimestampType.withoutZone(); | ||
| } | ||
| } else if (logical instanceof LogicalTypes.LocalTimestampMillis |
There was a problem hiding this comment.
LogicalTypes.LocalTimestamp{Millis,Micros} to TimestampType.withoutZone() and LocalTimestampNanos to TimestampNanoType.withoutZone().
| public static boolean isTimestamptz(Schema schema) { | ||
| LogicalType logicalType = schema.getLogicalType(); | ||
| if (logicalType instanceof LogicalTypes.LocalTimestampMillis |
There was a problem hiding this comment.
isTimestamptz returns false for LocalTimestamp* a
| case "timestamp-micros": | ||
| case "timestamp-nanos": | ||
| // both are handled in memory as long values, using the type to track units | ||
| case "local-timestamp-micros": |
| return GenericReaders.timestamps(); | ||
| case "local-timestamp-micros": | ||
| return GenericReaders.timestamps(); |
| } | ||
| return GenericReaders.timestampMillis(); | ||
| case "local-timestamp-millis": |
| Notes: | ||
| 1. Avro type annotation `adjust-to-utc` is an Iceberg convention; default value is `false` if not present. | ||
| 1. Non-zone timestamp types are written using Avro's `local-timestamp-{micros,nanos}` logical types. Zone-adjusted timestamp types are written using `timestamp-{micros,nanos}` with `adjust-to-utc: true`. Readers must also accept legacy files that encode non-zone timestamps as `timestamp-{micros,nanos}` with `adjust-to-utc: false`. |
There was a problem hiding this comment.
Following the Avro spec without any confusion.
There was a problem hiding this comment.
Let's back this out into a separate PR and do it first. We just had a lot of discussion on a similar issue for "dates" transform and I think we should have a discussion on this to make sure we aren't doing something that will break other implementations.
I would recommend starting a thread on the dev list and point to a specific PR for this change
There was a problem hiding this comment.
Thanks @RussellSpitzer for checking.
Here is the discussion thread: https://lists.apache.org/thread/jncwontk4xkmt7n5ml0pbgk4x54cwzgo
| private static final Schema TS_SCHEMA = | ||
| new Schema( | ||
| Types.NestedField.required(1, "ts", Types.TimestampType.withoutZone()), |
There was a problem hiding this comment.
Iceberg schema with all four timestamp variants (ts, tstz, ts_ns, tstz_ns)
| File file = temp.resolve("timestamps-" + System.nanoTime() + ".avro").toFile(); | ||
| try (org.apache.iceberg.io.FileAppender<Record> writer = | ||
| Avro.write(Files.localOutput(file)) |
There was a problem hiding this comment.
writes a single record holding LocalDateTime and OffsetDateTime values for all four fields
| List<Record> read; | ||
| try (AvroIterable<Record> reader = | ||
| Avro.read(Files.localInput(file)) |
There was a problem hiding this comment.
reads them back via Avro.read(...).createResolvingReader(PlannedDataReader::create)
bf9289a to
8c82bc8Compare8c82bc8 to
9248748Compare9248748 to
98aa875Compare| private static org.apache.avro.Schema rewriteLocalTimestampForFlink( | ||
| org.apache.avro.Schema schema) { | ||
| switch (schema.getType()) { |
There was a problem hiding this comment.
Avro schemas are nested: RECORD → FIELD → SCHEMA → UNION → ARRAY/MAP element → primitive.
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
Closes#12751
Switches Iceberg's Avro encoding for non-zone timestamp types
(
timestamp,timestamp_ns) to the Avro-spec-correctlocal-timestamp-{micros,nanos}logical types, instead of overloadingtimestamp-{micros,nanos}with the Iceberg-privateadjust-to-utc=falseconvention.The zone-adjusted variants (
timestamptz,timestamptz_ns) continueto use
timestamp-{micros,nanos}withadjust-to-utc=true.Why
Per the Avro spec,
timestamp-{millis,micros,nanos}is an instant onthe global timeline (UTC), while
local-timestamp-{millis,micros,nanos}is the timezone-free local timestamp.
Iceberg's writer mapped both the
zone and non-zone Iceberg types to
timestamp-*and disambiguated withthe
adjust-to-utcproperty, which the spec footnote already callsout as an Iceberg-only convention.