hudi-agent commented on code in PR #19733:
URL: https://github.com/apache/hudi/pull/19733#discussion_r3868563978
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/AlterTableCommand.scala:
##########
@@ -208,10 +211,98 @@ case class AlterTableCommand(table: CatalogTable,
changes: Seq[TableChange], cha
val newTable = table.copy(
properties = table.properties ++ properties,
comment =
properties.get(TableCatalog.PROP_COMMENT).orElse(table.comment))
+ updateHoodieTableConfigs(sparkSession, properties)
catalog.alterTable(newTable)
logInfo("table properties change finished")
}
+ /**
Review Comment:
🤖 nit: this Scaladoc block describes the update/delete methods' design
intent (alias mapping, catalog-only for non-Hudi props, etc.), but it's placed
directly above `protectedTableConfigs` so readers will associate it with the
val — could you move it to sit above `updateHoodieTableConfigs` where it
actually applies?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/AlterTableCommand.scala:
##########
@@ -208,10 +211,98 @@ case class AlterTableCommand(table: CatalogTable,
changes: Seq[TableChange], cha
val newTable = table.copy(
properties = table.properties ++ properties,
comment =
properties.get(TableCatalog.PROP_COMMENT).orElse(table.comment))
+ updateHoodieTableConfigs(sparkSession, properties)
catalog.alterTable(newTable)
logInfo("table properties change finished")
}
+ /**
+ * Persists Hudi table properties in hoodie.properties as well as the Spark
catalog.
+ *
+ * The analyzer only creates this command for Hudi V2 tables, so the table
is known to be a Hudi
+ * table. Keep non-Hudi properties catalog-only, matching Spark's normal
ALTER TABLE behavior.
+ * Hudi's SQL aliases and datasource options are converted to the canonical
keys stored in
+ * hoodie.properties before validation and persistence.
+ */
+ private val protectedTableConfigs = Set(
Review Comment:
🤖 nit: the field is called `protectedTableConfigs` but the thrown error
messages say "immutable table config" / "immutable table property" — could you
rename it to `immutableTableConfigs` so the two align? Also, since this set
doesn't depend on instance state, it might be cleaner in the companion object
to avoid reallocating it per instance.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
