wombatu-kun commented on code in PR #19810:
URL: https://github.com/apache/hudi/pull/19810#discussion_r3930364615
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaCompatibilityChecker.java:
##########
@@ -357,24 +357,24 @@ private SchemaCompatibilityResult
calculateCompatibility(final HoodieSchema read
case DATE:
case DECIMAL:
return result.mergedWith(typeMismatch(reader, writer, locations));
+ // TIMESTAMP over LONG and UUID over STRING are reader/writer
compatibility rules only. They are deliberately
+ // absent from HoodieSchemaTypePromotion: the projection checker
must not treat a bare long as a compatible
Review Comment:
`isCompatibleProjectionOf(source, target)` tests `canPromote(target,
source)`, so the entry kept out of the table is the one that would make a
timestamp a compatible projection of a bare long - this has it the other way
round. Could the direction be flipped here and in the matching sentence on
`HoodieSchemaTypePromotion`?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaCompatibility.java:
##########
@@ -701,6 +703,155 @@ public void testIsSchemaCompatibleWithTypePromotion() {
assertFalse(HoodieSchemaCompatibility.isSchemaCompatible(longS, intS,
true, true));
}
+ /**
+ * Sibling of {@link #testIsSchemaCompatibleWithTypePromotion()} covering
the rest of the reader/writer type
+ * table: the primitive widening cases shared with {@link
HoodieSchemaTypePromotion}, plus the two
+ * logical-type-over-primitive rules (TIMESTAMP over LONG, UUID over STRING)
that are compatibility-only.
+ *
+ * <p>All pairs are asserted through the 4-arg {@code
isSchemaCompatible(prev = writer, new = reader, true, true)}.</p>
+ */
+ @Test
+ public void testIsSchemaCompatibleWithLogicalTypesAndWidening() {
+ // Logical type over its backing primitive: accepted for reader/writer
compatibility.
+ assertCompatible(HoodieSchema.createTimestampMillis(),
HoodieSchema.create(HoodieSchemaType.LONG));
+ assertCompatible(HoodieSchema.createUUID(),
HoodieSchema.create(HoodieSchemaType.STRING));
+
+ // ... but only in that direction, and only over the matching primitive.
+ assertIncompatible(HoodieSchema.create(HoodieSchemaType.LONG),
HoodieSchema.createTimestampMillis());
+ assertIncompatible(HoodieSchema.createTimestampMillis(),
HoodieSchema.create(HoodieSchemaType.INT));
+ // DATE has no such rule at all, even though it is backed by INT.
+ assertIncompatible(HoodieSchema.createDate(),
HoodieSchema.create(HoodieSchemaType.INT));
+
+ // Primitive widening, delegated to HoodieSchemaTypePromotion.
+ assertCompatible(HoodieSchema.create(HoodieSchemaType.DOUBLE),
HoodieSchema.create(HoodieSchemaType.FLOAT));
+ assertIncompatible(HoodieSchema.create(HoodieSchemaType.FLOAT),
HoodieSchema.create(HoodieSchemaType.DOUBLE));
+ // The narrowing direction is rejected for every numeric pair. The
hudi-spark guard for the reversed
+ // argument bug class (TestTableSchemaEvolution, HUDI-1493) never runs, so
the pairs are pinned here.
+ assertIncompatible(HoodieSchema.create(HoodieSchemaType.INT),
HoodieSchema.create(HoodieSchemaType.LONG));
+ assertIncompatible(HoodieSchema.create(HoodieSchemaType.INT),
HoodieSchema.create(HoodieSchemaType.FLOAT));
+ assertIncompatible(HoodieSchema.create(HoodieSchemaType.INT),
HoodieSchema.create(HoodieSchemaType.DOUBLE));
+ assertIncompatible(HoodieSchema.create(HoodieSchemaType.LONG),
HoodieSchema.create(HoodieSchemaType.FLOAT));
+ assertIncompatible(HoodieSchema.create(HoodieSchemaType.LONG),
HoodieSchema.create(HoodieSchemaType.DOUBLE));
+ assertCompatible(HoodieSchema.create(HoodieSchemaType.STRING),
HoodieSchema.create(HoodieSchemaType.BYTES));
+ assertCompatible(HoodieSchema.create(HoodieSchemaType.BYTES),
HoodieSchema.create(HoodieSchemaType.STRING));
+ assertCompatible(HoodieSchema.create(HoodieSchemaType.STRING),
HoodieSchema.create(HoodieSchemaType.INT));
+ }
+
+ @Test
+ public void testAreSchemasCompatibleReaderIsFirstArgument() {
+ HoodieSchema longRecord = HoodieSchemaTestUtils.createRecord("R",
HoodieSchemaField.of("f", HoodieSchema.create(HoodieSchemaType.LONG), null,
null));
+ HoodieSchema intRecord = HoodieSchemaTestUtils.createRecord("R",
HoodieSchemaField.of("f", HoodieSchema.create(HoodieSchemaType.INT), null,
null));
+
+ // A long reader can read int data ...
+ assertTrue(HoodieSchemaCompatibility.areSchemasCompatible(longRecord,
intRecord));
+ // ... but not the other way round, which pins the reader as the FIRST
argument.
+ assertFalse(HoodieSchemaCompatibility.areSchemasCompatible(intRecord,
longRecord));
+ }
+
+ @Test
+ public void testLookupWriterFieldDirectMatch() {
+ HoodieSchemaField readerField = readerFieldWithAlias();
+ HoodieSchema writerSchema =
HoodieSchema.parse("{\"type\":\"record\",\"name\":\"W\",\"fields\":["
+ + "{\"name\":\"a\",\"type\":\"int\"}]}");
+
+ HoodieSchemaField writerField =
HoodieSchemaCompatibility.lookupWriterField(writerSchema, readerField);
+ assertEquals("a", writerField.name());
+ }
+
+ /**
+ * The alias path is also driven end to end through the sole production
caller,
+ * {@code HoodieTable#validateSchema} (see {@code
TestHoodieTableSchemaEvolution#testFieldWithAlias}); this
Review Comment:
`testFieldWithAlias` passes whether or not the alias resolves: a null lookup
short-circuits the `writerField != null` guard in
`HoodieTable#validateSecondaryIndexSchemaEvolution`, which is also the actual
caller rather than `validateSchema`. Could the end-to-end claim come out, or
that test gain a type change so the alias match is what decides it?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchema.java:
##########
@@ -75,6 +75,17 @@ public class TestHoodieSchema {
+ " ]"
+ "}";
+ // The Blob schema is persisted in the table schema of every BLOB table, so
its serialized shape is pinned literally.
+ private static final String BLOB_SCHEMA_JSON =
"{\"type\":\"record\",\"name\":\"blob\",\"fields\":["
Review Comment:
`BLOB_SCHEMA_JSON` and the existing `BLOB_JSON` in this class hold the same
string, one the expected literal and one computed from `createBlob()`. Could
the new one carry an `EXPECTED_` prefix so an assertion cannot pick up the
self-referential constant by mistake?
--
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]
