voonhous commented on code in PR #19029:
URL: https://github.com/apache/hudi/pull/19029#discussion_r3664724266
##########
hudi-common/src/test/java/org/apache/hudi/internal/schema/utils/TestAvroSchemaEvolutionUtils.java:
##########
@@ -563,8 +563,68 @@ public void
testNotEvolveSchemaIfReconciledSchemaUnchanged() {
InternalSchema oldInternalSchema =
InternalSchemaConverter.convert(oldSchema);
// set a non-default schema id for old table schema, e.g., 2.
oldInternalSchema.setSchemaId(2);
- InternalSchema evolvedSchema =
AvroSchemaEvolutionUtils.reconcileSchema(incomingSchema.getAvroSchema(),
oldInternalSchema, false);
+ InternalSchema evolvedSchema =
AvroSchemaEvolutionUtils.reconcileSchema(incomingSchema.getAvroSchema(),
oldInternalSchema, false, false);
// the evolved schema should be the old table schema, since there is no
type change at all.
Assertions.assertEquals(oldInternalSchema, evolvedSchema);
}
+
+ @Test
+ public void testReconcileSchemaTimestampPrecisionEvolution() {
Review Comment:
Added, and it turned out to matter more than a missing assertion.
`reconcileSchema` did reject cross-zone via `isTypeUpdateAllow`, but
`reconcileTimestampLogicalType` only checked `isGatedTimestampChange`, which is
`false` for a UTC/local pair, so it skipped the field entirely. That guard is
the only one on the default non-reconcile path, and the Avro reader/writer
check after it is logical-type-blind for two long-backed fields -- so a
`timestamp-micros` table silently became `local-timestamp-micros` on the next
commit. Same silent relabel this whole change set exists to prevent.
Wrote the test first and it failed exactly there. Now fixed in the guard: a
zone change throws with its own message, and no override unlocks it, since no
rescale expresses it. `testCrossZoneTimestampChangeIsRejected` covers both
entry points, both directions, override-can't-unlock, zone+precision together,
and a same-zone control.
##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieCommonConfig.java:
##########
@@ -83,6 +83,15 @@ public class HoodieCommonConfig extends HoodieConfig {
+ " operation will fail schema compatibility check. Set this option
to true will make the missing "
+ " column be filled with null values to successfully complete the
write operation.");
+ public static final ConfigProperty<Boolean>
ALLOW_TIMESTAMP_PRECISION_EVOLUTION = ConfigProperty
+ .key("hoodie.write.schema.allow.timestamp.precision.evolution")
+ .defaultValue(false)
+ .markAdvanced()
+ .sinceVersion("1.3.0")
+ .withDocumentation("Controls whether schema evolution may change a
column between timestamp-millis and "
Review Comment:
Good catch, and this was still a real gap. The `long -> local-timestamp`
attach did not get dropped -- it is implemented and tested: the `LONG` branch
in `isTypeUpdateAllowInternal`, the `LONG -> local` case in
`isGatedTimestampChange`, and the `local_ts_millis` / `local_ts_micros` entries
in the deltastreamer repair fixtures all exercise it. So the doc needed to
describe it rather than the code staying silent about it.
The config documentation now covers the attach case explicitly, plus the one
boundary it does not cross: a UTC/local zone change is never authorized by this
config, whatever the entry says.
--
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]
