voonhous commented on code in PR #19384:
URL: https://github.com/apache/hudi/pull/19384#discussion_r3673426470
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/internal/utils/TestAvroSchemaEvolutionUtils.java:
##########
@@ -840,4 +841,103 @@ public void testCrossZoneTimestampChangeIsRejected() {
SchemaChangeUtils.parseTimestampLogicalTypeOverrides("ts:local-timestamp-millis")).toAvroSchema();
Assertions.assertEquals("local-timestamp-millis",
stillWorks.getField("ts").schema().getLogicalType().getName());
}
+
+ @Test
+ void testLongToUtcTimestampGatedInBothReconcilePaths() {
+ // Bare long to a UTC timestamp is override-gated exactly like the
local-timestamp case: rejected
+ // without a per-field override and applied with one, in both reconcile
paths. The non-reconcile
+ // guard previously skipped this and let it through silently on the
default write path.
+ HoodieSchema tableBareLong =
HoodieSchema.fromAvroSchema(tripAvro(Schema.create(Schema.Type.LONG)));
+ HoodieSchema incomingMicros =
HoodieSchema.fromAvroSchema(tripAvro(LogicalTypes.timestampMicros().addToSchema(Schema.create(Schema.Type.LONG))));
+
+ // No override: rejected in both paths with the exact actionable error.
+ Map<String, Type> noOverride =
SchemaChangeUtils.parseTimestampLogicalTypeOverrides("");
+ String expectedError =
AvroSchemaEvolutionUtils.timestampPrecisionChangeError(
+ "ts", Types.LongType.get(), Types.TimestampType.get()).getMessage();
+ SchemaCompatibilityException reconcileError =
assertThrows(SchemaCompatibilityException.class,
+ () -> AvroSchemaEvolutionUtils.reconcileSchema(incomingMicros,
tableBareLong, false, noOverride));
+ assertEquals(expectedError, reconcileError.getMessage());
+ SchemaCompatibilityException guardError =
assertThrows(SchemaCompatibilityException.class,
+ () ->
AvroSchemaEvolutionUtils.reconcileTimestampLogicalType(incomingMicros,
tableBareLong, noOverride));
+ assertEquals(expectedError, guardError.getMessage());
+
+ // With the override: the promotion is applied in both paths.
+ Schema viaReconcile =
AvroSchemaEvolutionUtils.reconcileSchema(incomingMicros, tableBareLong, false,
+
SchemaChangeUtils.parseTimestampLogicalTypeOverrides("ts:timestamp-micros")).toAvroSchema();
+ assertEquals("timestamp-micros",
viaReconcile.getField("ts").schema().getLogicalType().getName());
+ Schema viaGuard =
AvroSchemaEvolutionUtils.reconcileTimestampLogicalType(incomingMicros,
tableBareLong,
+
SchemaChangeUtils.parseTimestampLogicalTypeOverrides("ts:timestamp-micros")).toAvroSchema();
+ assertEquals("timestamp-micros",
viaGuard.getField("ts").schema().getLogicalType().getName());
+ }
+
+ @Test
+ void testLongToLocalTimestampGatedInBothReconcilePaths() {
+ // Bare long to local timestamp is override-gated (not forbidden):
rejected without an override
+ // and applied with one, and the non-reconcile guard must agree with
reconcileSchema.
+ HoodieSchema tableBareLong =
HoodieSchema.fromAvroSchema(tripAvro(Schema.create(Schema.Type.LONG)));
+ HoodieSchema incomingLocalMicros =
HoodieSchema.fromAvroSchema(tripAvro(LogicalTypes.localTimestampMicros().addToSchema(Schema.create(Schema.Type.LONG))));
+
+ // No override: rejected in both paths with the exact actionable error.
+ Map<String, Type> noOverride =
SchemaChangeUtils.parseTimestampLogicalTypeOverrides("");
+ String expectedError =
AvroSchemaEvolutionUtils.timestampPrecisionChangeError(
+ "ts", Types.LongType.get(),
Types.LocalTimestampMicrosType.get()).getMessage();
+ SchemaCompatibilityException reconcileError =
assertThrows(SchemaCompatibilityException.class,
+ () -> AvroSchemaEvolutionUtils.reconcileSchema(incomingLocalMicros,
tableBareLong, false, noOverride));
+ assertEquals(expectedError, reconcileError.getMessage());
+ SchemaCompatibilityException guardError =
assertThrows(SchemaCompatibilityException.class,
+ () ->
AvroSchemaEvolutionUtils.reconcileTimestampLogicalType(incomingLocalMicros,
tableBareLong, noOverride));
+ assertEquals(expectedError, guardError.getMessage());
+ Schema repaired =
AvroSchemaEvolutionUtils.reconcileTimestampLogicalType(incomingLocalMicros,
tableBareLong,
+
SchemaChangeUtils.parseTimestampLogicalTypeOverrides("ts:local-timestamp-micros")).toAvroSchema();
+ assertEquals("local-timestamp-micros",
repaired.getField("ts").schema().getLogicalType().getName());
+ }
+
+ @Test
+ void testNestedLongToTimestampGated() {
+ // The gate resolves fully-qualified column names, so it applies to nested
fields too. A nested
+ // long -> timestamp (UTC or local) is override-gated via the dotted-key
override.
+ for (String token : new String[] {"timestamp-micros",
"local-timestamp-millis"}) {
Review Comment:
Test nit:
`local-timestamp-millis` here tests pre-existing behaviour -- narrow the
loop to just that token, run it against `d6ca44ed1279^`, and you get `Tests
run: 2, Failures: 0`. Only the `timestamp-micros` iteration is new.
Dropping it makes the loop single-token, which lets you delete `logicalLong`
entirely and inline the one type you need -- that also resolves the naming nit
the bot raised, better than renaming it. `HoodieSchema.createTimestampMicros()`
(`HoodieSchema.java:684`) is right there if you want a factory. Similarly
`nestedTrip` re-implements `HoodieSchemaTestUtils.createRecord` +
`createNestedField` from the same module.
Separately, worth spending the lines you save here on array and map
elements. I checked and the gate does reach them --
`InternalSchema.getAllColsFullName()` emits `arr.element` and `mp.value`, and
the dotted override applies on both paths:
```
cols=[arr, arr.element, mp, mp.value, mp.key, id]
guard threw: ... column 'arr.element' from 'long' to 'timestamp' ...
guard out arr =
{"type":"array","items":{"type":"long","logicalType":"timestamp-micros"}}
guard out mp =
{"type":"map","values":{"type":"long","logicalType":"timestamp-micros"}}
```
So the behaviour is fine, but nothing pins it and nothing tells a user that
an array element is addressed as `arr.element` and a map value as `mp.value`.
Please add those two cases here and one sentence naming the `element` / `value`
path segments in the config doc.
--
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]
