Re: [PR] fix(spark): make new schema evolution fields nullable [hudi]

["danny0405 (via GitHub)" <gi...@apache.org>]

danny0405 commented on code in PR #19665:
URL: https://github.com/apache/hudi/pull/19665#discussion_r3821531974


##########
hudi-common/src/main/java/org/apache/hudi/common/schema/internal/utils/AvroSchemaEvolutionUtils.java:
##########
@@ -332,7 +333,8 @@ public static HoodieSchema reconcileSchemaRequirements(HoodieSchema sourceSchema
     List<String> typeUpdateColsInSource = new ArrayList<>();
     colNamesSourceSchema.forEach(field -> {
       // handle columns that needs to be made nullable
-      if (colNamesTargetSchema.contains(field) && sourceInternalSchema.findField(field).isOptional() != targetInternalSchema.findField(field).isOptional()) {
+      if ((makeNewColumnsNullable && !colNamesTargetSchema.contains(field))

Review Comment:
   Addressed in 8e1ba38b300c. New paths are sorted and tracked with the same parent-visited guard used by `reconcileSchema`, which relaxes only the topmost field of a wholly new subtree. The regression test adds a new struct and array, asserts that each container is nullable with a null default, and asserts that the struct field, array element, and element field remain required.



##########
hudi-common/src/main/java/org/apache/hudi/common/schema/internal/utils/AvroSchemaEvolutionUtils.java:
##########
@@ -332,7 +333,8 @@ public static HoodieSchema reconcileSchemaRequirements(HoodieSchema sourceSchema
     List<String> typeUpdateColsInSource = new ArrayList<>();
     colNamesSourceSchema.forEach(field -> {
       // handle columns that needs to be made nullable
-      if (colNamesTargetSchema.contains(field) && sourceInternalSchema.findField(field).isOptional() != targetInternalSchema.findField(field).isOptional()) {
+      if ((makeNewColumnsNullable && !colNamesTargetSchema.contains(field))

Review Comment:
   Addressed in 8e1ba38b300c. New-field and existing-nullability candidates now exclude `META_FIELD_NAMES`, so an incoming metadata field is not treated as a user schema addition after the table schema has stripped metadata. `testMetadataFieldsAreExcludedFromNewColumnNullability` covers `_hoodie_commit_time` and verifies that it remains required.



##########
hudi-common/src/main/java/org/apache/hudi/common/config/HoodieCommonConfig.java:
##########
@@ -77,11 +77,14 @@ public class HoodieCommonConfig extends HoodieConfig {
   public static final ConfigProperty<String> SET_NULL_FOR_MISSING_COLUMNS = ConfigProperty
       .key("hoodie.write.set.null.for.missing.columns")
       .defaultValue("false")
+      .withAlternatives("hoodie.datasource.write.new.columns.nullable")

Review Comment:
   Agreed and addressed in 8e1ba38b300c. I removed the compatibility alias and all coupling to `hoodie.write.set.null.for.missing.columns`. Newly evolved fields are made nullable by default during canonicalization, while that config remains limited to existing table columns missing from the incoming batch. The functional test now runs without either null-backfill key in both schema-on-read modes.



##########
hudi-common/src/main/java/org/apache/hudi/common/schema/internal/utils/AvroSchemaEvolutionUtils.java:
##########
@@ -363,5 +365,9 @@ public static HoodieSchema reconcileSchemaRequirements(HoodieSchema sourceSchema
 
     return convert(SchemaChangeUtils.applyTableChanges2Schema(sourceInternalSchema, schemaChange), sourceSchema.getFullName());
   }
-}
 
+  public static HoodieSchema reconcileSchemaRequirements(HoodieSchema sourceSchema, HoodieSchema targetSchema,

Review Comment:
   Addressed in 8e1ba38b300c. After separating new-field evolution from the missing-column config, the temporary four-argument overload is no longer needed and has been removed. The original three-argument method is again the single API, so the caller/callee ordering issue is eliminated.



-- 
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: commits-unsubscribe@hudi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org