Copilot commented on code in PR #19733:
URL: https://github.com/apache/hudi/pull/19733#discussion_r3855957917
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/AlterTableCommand.scala:
##########
@@ -208,10 +210,57 @@ 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 def updateHoodieTableConfigs(sparkSession: SparkSession, properties:
Map[String, String]): Unit = {
+ val tableConfigs = HoodieOptionConfig.mapSqlOptionsToTableConfigs(
+ HoodieOptionConfig.extractHoodieOptions(properties))
+
+ if (tableConfigs.nonEmpty) {
+ val metaClient = getMetaClient(sparkSession)
+
+ HoodieWriterUtils.validateTableConfig(
+ sparkSession,
+ tableConfigs,
+ metaClient.getTableConfig)
+
+ HoodieTableConfig.update(
+ metaClient.getStorage,
+ metaClient.getMetaPath,
+ TypedProperties.fromMap(tableConfigs.asJava))
+ }
+ }
+ private def deleteHoodieTableConfigs(sparkSession: SparkSession,
propertyKeys: Seq[String]): Unit = {
+ val tableConfigs = HoodieOptionConfig.mapSqlOptionsToTableConfigs(
+ HoodieOptionConfig.extractHoodieOptions(propertyKeys.map(_ -> "").toMap))
+
+ if (tableConfigs.nonEmpty) {
+ val metaClient = getMetaClient(sparkSession)
+ HoodieTableConfig.delete(
+ metaClient.getStorage,
+ metaClient.getMetaPath,
+ tableConfigs.keySet.asJava)
+ }
Review Comment:
`deleteHoodieTableConfigs` deletes keys from `hoodie.properties` without any
validation. This allows `ALTER TABLE ... UNSET TBLPROPERTIES` to remove
runtime-immutable configs (for example, `hoodie.meta.fields.mode`) and bypass
the config immutability/conflict checks enforced elsewhere, potentially leaving
the table in an unsupported state.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/AlterTableCommand.scala:
##########
@@ -208,10 +210,57 @@ 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:
This change adds side effects on `hoodie.properties` for SET/UNSET
TBLPROPERTIES, but there is no coverage here ensuring `hoodie.properties` is
updated/deleted accordingly (and that non-Hudi properties remain catalog-only).
Existing `TestAlterTable` does not exercise SET/UNSET TBLPROPERTIES.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/spark/sql/hudi/command/AlterTableCommand.scala:
##########
@@ -208,10 +210,57 @@ 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 def updateHoodieTableConfigs(sparkSession: SparkSession, properties:
Map[String, String]): Unit = {
+ val tableConfigs = HoodieOptionConfig.mapSqlOptionsToTableConfigs(
+ HoodieOptionConfig.extractHoodieOptions(properties))
+
+ if (tableConfigs.nonEmpty) {
+ val metaClient = getMetaClient(sparkSession)
+
+ HoodieWriterUtils.validateTableConfig(
+ sparkSession,
+ tableConfigs,
+ metaClient.getTableConfig)
+
Review Comment:
`updateHoodieTableConfigs` calls
`HoodieWriterUtils.validateTableConfig(...)`, which rejects any SET of a
table-config key when the key already exists in `hoodie.properties` with a
different value. That means `ALTER TABLE ... SET TBLPROPERTIES` cannot actually
*change* existing persisted Hudi table configs (it will throw instead), which
conflicts with the PR description that these properties are now
persisted/changed via ALTER TABLE.
--
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]
