RussellSpitzer commented on PR #17196: URL: https://github.com/apache/iceberg/pull/17196#issuecomment-5273023048
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 and it's also somewhat moot since V4 manifests are no longer constrained to Avro, so that encoding question may not arise there.) 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. 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)); ``` That way the exception is visible and explicit where it matters, rather than a hidden mode in the utility. The utility should only take a mode flag when the output is genuinely ambiguous from the input alone and local-timestamp-* is unambiguous. If you want to throw an exception when adjust-to-utc is not a boolean that sounds fine to me. Generally we would only do this at the moment of conversion though so make sure it's not a parse error but an error when we are trying to do something with the type that is undefined. -- 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]
