voonhous commented on code in PR #19810:
URL: https://github.com/apache/hudi/pull/19810#discussion_r3923833786
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -248,36 +251,42 @@ public static HoodieSchema mergeSchemas(HoodieSchema
sourceSchema, HoodieSchema
}
/**
- * Creates a nullable version of the given schema (union of null and the
schema).
+ * Create a new schema by force changing all the top-level fields as
nullable.
*
- * <p>{@link HoodieSchema#createNullable(HoodieSchema)} is the idempotent
native equivalent and is
- * preferred; this overload round-trips through Avro and is retained only
for existing call sites.</p>
+ * <p>The rewrite runs through the field-id {@link InternalSchema}: the
record is converted, every
+ * still-required top-level field is marked nullable with a {@link
TableChanges.ColumnUpdateChange},
+ * and the updated InternalSchema is converted back under the original full
name. Only the top level
+ * changes - the inner fields of a nested record keep the nullability they
had. Because the record is
+ * rebuilt from the InternalSchema, its full name, field order and per-field
docs survive, while the
+ * record-level doc and any custom record properties do not.</p>
*
- * @param schema the input schema
- * @return new HoodieSchema that allows null values
- * @throws IllegalArgumentException if schema is null
- */
- public static HoodieSchema createNullableSchema(HoodieSchema schema) {
- ValidationUtils.checkArgument(schema != null, "Schema cannot be null");
-
- // Delegate to AvroSchemaUtils
- Schema nullableAvro =
AvroSchemaUtils.createNullableSchema(schema.toAvroSchema());
- return HoodieSchema.fromAvroSchema(nullableAvro);
- }
-
- /**
- * Create a new schema by force changing all the fields as nullable.
+ * <p>When every top-level field is already nullable the input instance
itself is returned and no
+ * conversion runs.</p>
*
- * @return a new schema with all the fields updated as nullable
+ * @param schema original schema
+ * @return a schema with all the top-level fields updated as nullable, or
{@code schema} itself when
+ * there is nothing to change
* @throws IllegalArgumentException if schema is null
- * @see AvroSchemaUtils#asNullable(Schema)
*/
public static HoodieSchema asNullable(HoodieSchema schema) {
ValidationUtils.checkArgument(schema != null, "Schema cannot be null");
- // Delegate to AvroSchemaUtils
- Schema nullableAvro = AvroSchemaUtils.asNullable(schema.toAvroSchema());
- return HoodieSchema.fromAvroSchema(nullableAvro);
+ // NOTE: HoodieSchema#isNullable is false for a bare NULL type, unlike
Avro's Schema#isNullable, so a
+ // NULL-typed field is excluded explicitly to keep it out of the
update list as it always was.
+ List<String> requiredCols = schema.getFields().stream()
+ .filter(f -> !(f.schema().isNullable() || f.schema().getType() ==
HoodieSchemaType.NULL))
+ .map(HoodieSchemaField::name)
+ .collect(Collectors.toList());
+ if (requiredCols.isEmpty()) {
+ return schema;
+ }
+
+ InternalSchema internalSchema = InternalSchemaConverter.convert(schema);
Review Comment:
**major:** pre-existing, not introduced by this rewrite: the deleted
`AvroSchemaUtils#asNullable` path gives the identical output.
`InternalSchemaConverter.buildBlobInternalRecordType()` gives the blob's nested
`reference` fields ids 0..3 and `InternalSchema.buildIdToField` is one flat
last-put-wins map, so on `{id: int, b: nullable BLOB}` this call returns a
record whose first field is `external_path` instead of `id`; a required BLOB
throws `Cannot update nullability for column 'b'`. Reachable from Flink
`ClusteringOperator.open()` on any BLOB table. Filed as #19833 with the repro.
Could we keep the fix out of this PR so the conversion stays as on master and
the PR remains a pure refactor?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaUtils.java:
##########
@@ -2084,6 +2086,140 @@ public void
testGetNestedFieldComplexNestedMapAndArray() {
assertEquals(HoodieSchemaType.LONG,
result.get().getRight().schema().getType());
}
+ /**
+ * Record with a namespace, a record-level doc, a custom record prop,
per-field docs and a nested
+ * record - all of its top-level fields required.
+ */
+ private static HoodieSchema allRequiredPersonSchema() {
+ HoodieSchema address = HoodieSchema.createRecord(
+ "Address",
+ "ns.test",
+ "the address record",
+ Arrays.asList(
+ HoodieSchemaField.of("city",
HoodieSchema.create(HoodieSchemaType.STRING), "city doc", null),
+ HoodieSchemaField.of("zip",
HoodieSchema.create(HoodieSchemaType.INT), null, null)));
+ HoodieSchema schema = HoodieSchema.createRecord(
+ "Person",
+ "ns.test",
+ "the person record",
+ Arrays.asList(
+ HoodieSchemaField.of("id",
HoodieSchema.create(HoodieSchemaType.INT), "id doc", null),
+ HoodieSchemaField.of("name",
HoodieSchema.create(HoodieSchemaType.STRING), null, null),
+ HoodieSchemaField.of("address", address, "address doc", null)));
+ schema.addProp("hoodie.custom.prop", "custom-value");
+ return schema;
+ }
+
+ @Test
+ public void testAsNullableMakesEveryTopLevelFieldNullable() {
Review Comment:
**minor:** not blocking, pre-existing behaviour. The new javadoc reads as
exhaustive, but the round trip also drops a non-null default to `null`
(`InternalSchemaConverter:534`), lowers ENUM to STRING (`:340`) and reorders a
null-last union null-first; the fixture at line 2171 is already null-first, so
it cannot show that. VECTOR survives, which is the Flink-realistic column.
Could we add one assertion each (a `"default": 0` field, an ENUM, a
`["string","null"]` fixture, a VECTOR column) and a javadoc clause for the
first two?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaCompatibility.java:
##########
@@ -701,6 +703,120 @@ 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));
Review Comment:
**minor:** not blocking. This is the only reverse-direction numeric pair
pinned here, and the hudi-spark guard for the reversed-argument bug class
(`TestTableSchemaEvolution:134`, from HUDI-1493) has never executed: no
`allowedFieldChanges` entry is `"byte"`, so its `assertFalse` is dead. Could we
add `assertIncompatible` for the other five narrowings (INT <-
LONG/FLOAT/DOUBLE, LONG <- FLOAT/DOUBLE) so a swapped `canPromote(reader,
writer)` at `HoodieSchemaCompatibilityChecker:376` fails here?
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaTypePromotion.java:
##########
@@ -19,7 +19,8 @@
package org.apache.hudi.common.schema;
/**
- * The single table of primitive widening promotions, used by {@link
HoodieSchemaProjectionChecker}.
+ * The single table of primitive widening promotions, used by {@link
HoodieSchemaProjectionChecker} and, for the
+ * primitive cases, by {@link HoodieSchemaCompatibilityChecker}.
Review Comment:
**nit:** feel free to ignore. This header now names
`HoodieSchemaCompatibilityChecker` as a second consumer, but line 46 below
still says the class is "used only by HoodieSchemaProjectionChecker". Could we
drop line 46, since this paragraph already says it?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaTypePromotion.java:
##########
@@ -86,6 +86,11 @@ public void testUnrelatedTypesNotPromotable() {
assertFalse(HoodieSchemaTypePromotion.canPromote(HoodieSchemaType.BOOLEAN,
HoodieSchemaType.INT));
assertFalse(HoodieSchemaTypePromotion.canPromote(HoodieSchemaType.INT,
HoodieSchemaType.BOOLEAN));
assertFalse(HoodieSchemaTypePromotion.canPromote(HoodieSchemaType.LONG,
HoodieSchemaType.STRING));
+ // Logical-type-over-primitive pairs are reader/writer compatibility rules
only (see
+ // HoodieSchemaCompatibilityChecker); they must never be reported as
compatible projections.
+
assertFalse(HoodieSchemaTypePromotion.canPromote(HoodieSchemaType.TIMESTAMP,
HoodieSchemaType.LONG));
+ assertFalse(HoodieSchemaTypePromotion.canPromote(HoodieSchemaType.UUID,
HoodieSchemaType.STRING));
+ assertFalse(HoodieSchemaTypePromotion.canPromote(HoodieSchemaType.DATE,
HoodieSchemaType.INT));
Review Comment:
**nit:** feel free to ignore. The invariant that keeps the LONG/FLOAT/DOUBLE
collapse safe for int-backed logical types is that `HoodieSchemaType` keeps
DATE/TIME distinct from INT; these lines pin the reader side only. Could we add
`assertFalse(canPromote(LONG, DATE))` and `assertFalse(canPromote(LONG, TIME))`
for the writer side?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaCompatibility.java:
##########
@@ -701,6 +703,120 @@ 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));
+ 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 =
singleFieldRecord(HoodieSchema.create(HoodieSchemaType.LONG));
+ HoodieSchema intRecord =
singleFieldRecord(HoodieSchema.create(HoodieSchemaType.INT));
+
+ // 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());
+ }
+
+ @Test
+ public void testLookupWriterFieldAliasMatch() {
Review Comment:
**nit:** feel free to ignore. The alias case is also covered end to end
through the sole production caller
(`TestHoodieTableSchemaEvolution#testFieldWithAlias` -> `HoodieTable:1000`);
the other four cases here are new. Could a one-line javadoc cross-reference
make the overlap deliberate?
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/HoodieSchemaUtils.java:
##########
@@ -248,36 +251,42 @@ public static HoodieSchema mergeSchemas(HoodieSchema
sourceSchema, HoodieSchema
}
/**
- * Creates a nullable version of the given schema (union of null and the
schema).
+ * Create a new schema by force changing all the top-level fields as
nullable.
*
- * <p>{@link HoodieSchema#createNullable(HoodieSchema)} is the idempotent
native equivalent and is
- * preferred; this overload round-trips through Avro and is retained only
for existing call sites.</p>
+ * <p>The rewrite runs through the field-id {@link InternalSchema}: the
record is converted, every
+ * still-required top-level field is marked nullable with a {@link
TableChanges.ColumnUpdateChange},
+ * and the updated InternalSchema is converted back under the original full
name. Only the top level
+ * changes - the inner fields of a nested record keep the nullability they
had. Because the record is
+ * rebuilt from the InternalSchema, its full name, field order and per-field
docs survive, while the
+ * record-level doc and any custom record properties do not.</p>
*
- * @param schema the input schema
- * @return new HoodieSchema that allows null values
- * @throws IllegalArgumentException if schema is null
- */
- public static HoodieSchema createNullableSchema(HoodieSchema schema) {
- ValidationUtils.checkArgument(schema != null, "Schema cannot be null");
-
- // Delegate to AvroSchemaUtils
- Schema nullableAvro =
AvroSchemaUtils.createNullableSchema(schema.toAvroSchema());
- return HoodieSchema.fromAvroSchema(nullableAvro);
- }
-
- /**
- * Create a new schema by force changing all the fields as nullable.
+ * <p>When every top-level field is already nullable the input instance
itself is returned and no
+ * conversion runs.</p>
*
- * @return a new schema with all the fields updated as nullable
+ * @param schema original schema
+ * @return a schema with all the top-level fields updated as nullable, or
{@code schema} itself when
+ * there is nothing to change
* @throws IllegalArgumentException if schema is null
- * @see AvroSchemaUtils#asNullable(Schema)
*/
public static HoodieSchema asNullable(HoodieSchema schema) {
ValidationUtils.checkArgument(schema != null, "Schema cannot be null");
- // Delegate to AvroSchemaUtils
- Schema nullableAvro = AvroSchemaUtils.asNullable(schema.toAvroSchema());
- return HoodieSchema.fromAvroSchema(nullableAvro);
+ // NOTE: HoodieSchema#isNullable is false for a bare NULL type, unlike
Avro's Schema#isNullable, so a
+ // NULL-typed field is excluded explicitly to keep it out of the
update list as it always was.
+ List<String> requiredCols = schema.getFields().stream()
Review Comment:
**minor:** not blocking. A non-record input now fails here with
`IllegalStateException` from `HoodieSchema#getFields` (`HoodieSchema:1178`)
where the deleted Avro path threw `AvroRuntimeException: Not a record`;
unreachable today (`ClusteringOperator:177` always passes a record), but the
type change is a side effect rather than a decision. Could we add
`checkArgument(schema.getType() == HoodieSchemaType.RECORD, ...)` with an
`@throws` line and a one-line `assertThrows`?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchemaCompatibility.java:
##########
@@ -701,6 +703,120 @@ 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));
+ 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 =
singleFieldRecord(HoodieSchema.create(HoodieSchemaType.LONG));
+ HoodieSchema intRecord =
singleFieldRecord(HoodieSchema.create(HoodieSchemaType.INT));
+
+ // 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());
+ }
+
+ @Test
+ public void testLookupWriterFieldAliasMatch() {
+ HoodieSchemaField readerField = readerFieldWithAlias();
+ HoodieSchema writerSchema =
HoodieSchema.parse("{\"type\":\"record\",\"name\":\"W\",\"fields\":["
+ + "{\"name\":\"old_a\",\"type\":\"int\"}]}");
+
+ HoodieSchemaField writerField =
HoodieSchemaCompatibility.lookupWriterField(writerSchema, readerField);
+ assertEquals("old_a", writerField.name());
+ }
+
+ @Test
+ public void testLookupWriterFieldAmbiguousMatchThrows() {
+ HoodieSchemaField readerField = readerFieldWithAlias();
+ HoodieSchema writerSchema =
HoodieSchema.parse("{\"type\":\"record\",\"name\":\"W\",\"fields\":["
+ + "{\"name\":\"a\",\"type\":\"int\"},"
+ + "{\"name\":\"old_a\",\"type\":\"int\"}]}");
+
+ assertThrows(HoodieSchemaException.class,
+ () -> HoodieSchemaCompatibility.lookupWriterField(writerSchema,
readerField));
+ }
+
+ @Test
+ public void testLookupWriterFieldNoMatchReturnsNull() {
+ HoodieSchemaField readerField = readerFieldWithAlias();
+ HoodieSchema writerSchema =
HoodieSchema.parse("{\"type\":\"record\",\"name\":\"W\",\"fields\":["
+ + "{\"name\":\"unrelated\",\"type\":\"int\"}]}");
+
+ assertNull(HoodieSchemaCompatibility.lookupWriterField(writerSchema,
readerField));
+ }
+
+ @Test
+ public void testLookupWriterFieldRejectsNonRecordWriterSchema() {
+ HoodieSchemaField readerField = readerFieldWithAlias();
+ HoodieSchema notARecord = HoodieSchema.create(HoodieSchemaType.STRING);
+
+ assertThrows(IllegalArgumentException.class,
+ () -> HoodieSchemaCompatibility.lookupWriterField(notARecord,
readerField));
+ }
+
+ /**
+ * Reader field {@code a}, aliased {@code old_a}. Aliases have no builder on
HoodieSchemaField, so the
+ * reader record is parsed from JSON.
+ */
+ private static HoodieSchemaField readerFieldWithAlias() {
+ HoodieSchema readerSchema =
HoodieSchema.parse("{\"type\":\"record\",\"name\":\"R\",\"fields\":["
+ + "{\"name\":\"a\",\"type\":\"int\",\"aliases\":[\"old_a\"]}]}");
+ return readerSchema.getField("a").get();
+ }
+
+ private static void assertCompatible(HoodieSchema readerFieldSchema,
HoodieSchema writerFieldSchema) {
+ assertTrue(HoodieSchemaCompatibility.isSchemaCompatible(
+ singleFieldRecord(writerFieldSchema),
singleFieldRecord(readerFieldSchema), true, true),
+ "reader " + readerFieldSchema + " should read writer " +
writerFieldSchema);
+ }
+
+ private static void assertIncompatible(HoodieSchema readerFieldSchema,
HoodieSchema writerFieldSchema) {
+ assertFalse(HoodieSchemaCompatibility.isSchemaCompatible(
+ singleFieldRecord(writerFieldSchema),
singleFieldRecord(readerFieldSchema), true, true),
+ "reader " + readerFieldSchema + " should not read writer " +
writerFieldSchema);
+ }
+
+ private static HoodieSchema singleFieldRecord(HoodieSchema fieldSchema) {
Review Comment:
**nit:** feel free to ignore. `singleFieldRecord` re-implements
`HoodieSchemaTestUtils.createRecord(String, HoodieSchemaField...)`
(`HoodieSchemaTestUtils:63-65`, same body). Could we call that helper instead?
##########
hudi-common/src/test/java/org/apache/hudi/common/schema/TestHoodieSchema.java:
##########
@@ -2552,6 +2554,26 @@ public void testCreateBlob() {
assertFalse(managedOpt.get().schema().isNullable());
}
+ @Test
+ public void testBlobNullableFieldsPutNullFirst() {
Review Comment:
**minor:** not blocking. This PR rewrote how the Blob fields are built, and
the Blob schema is what BLOB tables persist, but nothing pins its serialized
shape: a dropped `"default": null` on `data`/`reference`, a flipped union order
or a lost `logicalType` all pass the count and nullability checks. Could we
replace this loop with a literal pin in `testCreateBlob`,
`assertEquals(BLOB_JSON, HoodieSchema.createBlob().toAvroSchema().toString())`?
The current string, verified by running it:
```
{"type":"record","name":"blob","fields":[{"name":"type","type":{"type":"enum","name":"blob_storage_type","symbols":["INLINE","OUT_OF_LINE"]}},{"name":"data","type":["null","bytes"],"default":null},{"name":"reference","type":["null",{"type":"record","name":"reference","fields":[{"name":"external_path","type":"string"},{"name":"offset","type":["null","long"]},{"name":"length","type":["null","long"]},{"name":"managed","type":"boolean"}]}],"default":null}],"logicalType":"blob"}
```
--
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]
