wombatu-kun commented on code in PR #20492:
URL: https://github.com/apache/hudi/pull/20492#discussion_r4439091937
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/catalog/HoodieCatalog.scala:
##########
@@ -229,7 +230,20 @@ class HoodieCatalog extends DelegatingCatalogExtension
case _ => false
}
val catalogPropertyChanges = if (propertyChanges.isEmpty) Seq.empty
else catalogPropertyChangesToApply(table, propertyChanges)
- otherChanges.groupBy(c => c.getClass).foreach {
+ // SDP reconciles its business schema against the catalog's schema on
each refresh.
+ // Hudi owns its metadata columns; deleting them must not change the
stored schema.
+ val schemaChanges = otherChanges.filterNot {
+ case delete: DeleteColumn =>
+ delete.fieldNames().length == 1 &&
HoodieRecord.HOODIE_META_COLUMNS.contains(delete.fieldNames()(0))
+ case _ => false
+ }
+ // Reject unsupported changes before applying any supported change in
the same request.
+ schemaChanges.foreach {
+ case _: AddColumn | _: UpdateColumnType | _: UpdateColumnComment =>
Review Comment:
`UpdateColumnType` passes this check, but
`AlterHoodieTableChangeColumnCommand` rejects every real type change, so a
refresh that widens one column and adds another can commit the add and then
fail, depending on the `groupBy` order over classes. Could this check also
reject a real type change up front, or could type changes run before adds?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/catalog/HoodieCatalog.scala:
##########
@@ -229,7 +230,20 @@ class HoodieCatalog extends DelegatingCatalogExtension
case _ => false
}
val catalogPropertyChanges = if (propertyChanges.isEmpty) Seq.empty
else catalogPropertyChangesToApply(table, propertyChanges)
- otherChanges.groupBy(c => c.getClass).foreach {
+ // SDP reconciles its business schema against the catalog's schema on
each refresh.
+ // Hudi owns its metadata columns; deleting them must not change the
stored schema.
+ val schemaChanges = otherChanges.filterNot {
+ case delete: DeleteColumn =>
+ delete.fieldNames().length == 1 &&
HoodieRecord.HOODIE_META_COLUMNS.contains(delete.fieldNames()(0))
+ case _ => false
+ }
+ // Reject unsupported changes before applying any supported change in
the same request.
+ schemaChanges.foreach {
+ case _: AddColumn | _: UpdateColumnType | _: UpdateColumnComment =>
+ case change =>
Review Comment:
A materialized view column declared `NOT NULL` comes back nullable from
`loadTable` (`HoodieCatalogTable.loadTableSchemaByMetaClient` forces `nullable
= true`), so SDP sends `UpdateColumnNullability(col, false)` on every refresh
of an unchanged definition and this rejects it after the truncate. Should
nullability changes be a no-op here, as RFC-112's change matrix already asks
for loosening, or is rejecting this case intended?
--
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]
