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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
