yihua commented on code in PR #19665:
URL: https://github.com/apache/hudi/pull/19665#discussion_r3826223196
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/hudi/functional/TestCOWDataSource.scala:
##########
@@ -1979,35 +1980,39 @@ class TestCOWDataSource extends
HoodieSparkClientTestBase with ScalaAssertionSup
HoodieWriteConfig.KEYGENERATOR_CLASS_NAME.key() ->
"org.apache.hudi.keygen.ComplexKeyGenerator",
KeyGeneratorOptions.HIVE_STYLE_PARTITIONING_ENABLE.key() -> "true",
HiveSyncConfigHolder.HIVE_SYNC_ENABLED.key() -> "false",
- HoodieWriteConfig.RECORD_MERGE_IMPL_CLASSES.key() ->
"org.apache.hudi.DefaultSparkRecordMerger"
+ HoodieWriteConfig.RECORD_MERGE_IMPL_CLASSES.key() ->
"org.apache.hudi.DefaultSparkRecordMerger",
+ HoodieCommonConfig.RECONCILE_SCHEMA.key() -> "true",
Review Comment:
Could this test parameterize on this config as well? Is this config
required to reproduce the issue?
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/internal/utils/AvroSchemaEvolutionUtils.java:
##########
@@ -302,17 +302,19 @@ public static SchemaCompatibilityException
timestampPrecisionChangeError(String
* {@code target} one. Source is considered to be new incoming schema, while
target could refer to prev table schema.
* For example,
* if colA in source is non-nullable, but is nullable in target, output
schema will have colA as nullable.
- * if "hoodie.datasource.write.new.columns.nullable" is set to true and if
colB is not present in source, but
- * is present in target, output schema will have colB as nullable.
+ * if colB is present in source, but not in target, output schema will have
colB as nullable. If colB is a complex
+ * type, its existing descendants retain their nullability constraints.
Review Comment:
In the PR description we should make a note on updating the release notes
as this flips the default behavior: adding a required column used to fail with
`SchemaBackwardsCompatibilityException(READER_FIELD_MISSING_DEFAULT_VALUE)`;
after this PR is landed, the behavior is to widen the column to nullable.
##########
hudi-common/src/main/java/org/apache/hudi/common/schema/internal/utils/AvroSchemaEvolutionUtils.java:
##########
@@ -330,9 +332,28 @@ public static HoodieSchema
reconcileSchemaRequirements(HoodieSchema sourceSchema
List<String> nullableUpdateColsInSource = new ArrayList<>();
List<String> typeUpdateColsInSource = new ArrayList<>();
+
+ // Only relax the topmost field in a wholly new subtree. Relaxing every
descendant would alter the
+ // element/field constraints supplied by the writer instead of only making
the evolved field backfillable.
+ Set<String> visitedNewColumns = new HashSet<>();
+ colNamesSourceSchema.stream()
+ .filter(field -> !colNamesTargetSchema.contains(field))
+ .filter(field -> !META_FIELD_NAMES.contains(field))
+ .sorted()
+ .forEach(field -> {
+ String parent = TableChangesHelper.getParentName(field);
+ if (!visitedNewColumns.contains(parent)) {
+ nullableUpdateColsInSource.add(field);
+ }
+ visitedNewColumns.add(field);
+ });
+
colNamesSourceSchema.forEach(field -> {
- // handle columns that needs to be made nullable
- if (colNamesTargetSchema.contains(field) &&
sourceInternalSchema.findField(field).isOptional() !=
targetInternalSchema.findField(field).isOptional()) {
+ // Reconcile nullability only for existing user columns. Metadata fields
may be present in the source even
+ // though the target schema used for canonicalization has intentionally
stripped them.
+ if (colNamesTargetSchema.contains(field)
+ && !META_FIELD_NAMES.contains(field)
Review Comment:
nit: this condition is no longer required because `latestTableSchema` here
always has meta fields stripped, `colNamesTargetSchema.contains(field)` already
excludes them, so this extra guard can't fire from the current caller. If
being defensive, it is better to filter out meta fields from
`colNamesSourceSchema` and feed that to L339 and L354 for processing.
--
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]
