kinolaev commented on PR #17196: URL: https://github.com/apache/iceberg/pull/17196#issuecomment-5256109471
@RussellSpitzer, I reread your last message and realized that: > convert(Schema) ← no-arg, defaults adjustToUtcDefault=true most probably was a description of the branch state at that time, rather than a suggestion to keep `true` by default. That is why I updated the PR again, and I think it should now be very close to what you envisioned: 1. AvroSchemaUtil Iceberg->Avro conversion methods produce `local-timestamp-*` unconditionally, which is allowed by the updated spec 2. The `write.avro.local-timestamp.enabled` table property (`false` by default) controls Avro data file schemas 3. The `adjust-to-utc` default is now `false` everywhere (matches the spec) 4. AvroSchemaUtil Avro->Iceberg conversion methods have overloads to allow setting `adjustToUtcDefault=true` 5. `DataWriter` and readers allow setting `adjustToUtcDefault=true` via the `SupportsLocalTimestamp` interface 6. `isTimestamptz` has been inlined The main question right now is: is it acceptable to encode partition fields `identity(timestamp)`, `identity(timestamp_ns)`, `void(timestamp)` and `void(timestamp_ns)` as `local-timestamp-*` in manifests? A smaller question: is it OK to throw an exception when the `adjust-to-utc` value is not a boolean? PS: I found several more places in Flink where `legacyTimestampMapping` was still `true` by default and flipped them to `false` (d1c8e61edc29ab31a6e60e09b289813a087ee82b). -- 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]
