yihua commented on code in PR #19665:
URL: https://github.com/apache/hudi/pull/19665#discussion_r3818896124
##########
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:
This branch doesn’t exclude the meta fields the way reconcileSchema does
below. Since deduceWriterSchema strips meta fields from latestTableSchema but
not from sourceSchema, a write that keeps them in the incoming df (pk-less
prepped upsert/delete, streaming writes — HoodieSparkSqlWriter.scala:371) sees
_hoodie_commit_time as a new column and updateColumnNullability throws
`IllegalArgumentException: cannot modify hudi meta col: _hoodie_commit_time`
from checkColModifyIsLegal, before the no-op check can spare it. Could we
filter META_FIELD_NAMES here too?
##########
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:
For a column that’s entirely new, every descendant full-name also misses
`colNamesTargetSchema`, so the whole subtree gets relaxed — a new `items
array<struct<sku:string>>` comes out as `array<[null,
struct<sku:[null,string]>]>` rather than just the top-level field being made
nullable, and that widening can’t be undone later since
`updateColumnNullability` refuses optional -> required. `reconcileSchema` uses
the visited/parent guard below for this case; could we relax only the topmost
newly-added field, and add a test for a brand-new struct/array column?
##########
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:
These two configs are for different scenarios:
- "hoodie.write.set.null.for.missing.columns": the input has a narrower
schema than the table schema, i.e., the input schema does not have one or more
fields in the table schema; enabling the config allows the missing columns to
be `null`;
- "hoodie.datasource.write.new.columns.nullable": new fields added as part
of the schema evolution should be nullable. In fact, this should be made the
default behavior IMO, independent of
"hoodie.write.set.null.for.missing.columns", not controlled by
"hoodie.datasource.write.new.columns.nullable" anymore. Otherwise, adding a
non-null field as part of the schema evolution fails the reader.
"hoodie.datasource.write.new.columns.nullable" can still be turned off
regardless of adding nullable fields, because the user may still want the
ingestion to fail if missing columns in the input are an issue, but want the
schema evolution to succeed if evolving the table schema with non-null fields
is not an issue.
--
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]
