kinolaev commented on PR #17196:
URL: https://github.com/apache/iceberg/pull/17196#issuecomment-5316756720
> My preference is to keep the converter semantically correct (withoutZone()
always produces local-timestamp-micros) and push the legacy wire format
requirement to the callers that actually need it:
>
> ```java
> // Caller that is producing a schema that we want to write into AVRO
> manifestPartitionSchema =
toLegacyTimestamps(convert(icebergPartitionSchema));
> ```
I added package-private `AvroLegacyTimestamps.convert`, but I realized all 5
of its call sites are mostly dead code. Specifically: the `BaseFile` and
`GenericManifestEntry` constructors, `GenericDataFile.getAvroSchema`,
`GenericDeleteFile.getAvroSchema`, and `PartitionData.partitionDataSchema`.
They all generate an Avro schema solely to implement the `IndexedRecord`
interface, which is only used by the kafka-connect module via
`AvroEncoderUtil.encode`->`GenericAvroWriter` to serialize and deserialize
events in the control topic (except for `GenericManifestEntry`, whose
`IndexedRecord` support isn't used anywhere). Should we keep the local to
legacy timestamp conversion in these 5 classes?
> We can't change the Avro encoding in manifests from what the spec
currently says, and we can't change the spec for V3. (We could update V4 to
accept or require the new types, but that's a separate discussion...)
> The manifest writers aren't broken today because
legacyTimestampMapping=true is the default. But that's the problem: the flag is
in the wrong place. If we ever flip the default, or a caller adopts the new
non-legacy path, the manifest writers silently produce spec-incompatible output
with no indication at the call site that anything special is happening.
The actual schema is written via
`ManifestWriter.V{Version}(Delete)Writer`->`InternalData.write`->`Avro.write`.
That means it depends on the `write.avro.local-timestamp.enabled` property.
I've pinned it to `false` for all versions (including v4) and removed
`local-timestamp-*` from the spec for now. Once this PR merges into `main`, I
plan to open a follow-up PR about supporting `local-timestamp-*` in the spec.
Thanks @RussellSpitzer for the review! Could you please take another look at
the PR when you get a chance?
--
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]
