hudi-agent commented on code in PR #19733:
URL: https://github.com/apache/hudi/pull/19733#discussion_r3856931758
##########
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))
Review Comment:
🤖 nit: the `map(_ -> "").toMap` trick — using dummy empty values just to get
key translation — is a bit surprising on first read. A short inline comment
like `// dummy values; only the translated keys are used` would save the next
reader a minute of head-scratching.
<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 +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:
🤖 Agree this is worth guarding. `extractHoodieOptions` passes through any
`hoodie.*` key, so UNSET could delete immutable configs like
`hoodie.table.recordkey.fields`, `hoodie.table.name`, or
`hoodie.table.partition.fields` from hoodie.properties and leave the table
unreadable. Restricting UNSET to a known-mutable set (or routing it through the
same validation) rather than deleting arbitrary hoodie keys seems safer.
##########
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))
Review Comment:
🤖 `extractHoodieOptions` keeps every key starting with `hoodie.`, and
`mapSqlOptionsToTableConfigs` passes keys it doesn't recognize through
unchanged, so this looks like it will persist *any* `hoodie.*` property into
`hoodie.properties` — not just table configs. e.g. `SET
TBLPROPERTIES('hoodie.upsert.shuffle.parallelism'='100')` or a
`hoodie.datasource.write.*` key would get written there. Since
`validateTableConfig` only flags value *conflicts* with existing keys (a
brand-new key has `existingValue == null`), nothing filters these out, and
they'd become persistent table-level defaults. Could you restrict persistence
to actual `HoodieTableConfig` keys?
<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 +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:
🤖 Confirmed from the code — `validateTableConfig` appends to `diffConfigs`
and throws whenever an incoming key already exists in the table config with a
different value (HoodieWriterUtils around line 301). Only a small whitelist in
`shouldIgnoreConfig` (base file format, a few payload/merge-mode cases, empty
database name) is exempt, so any genuine change to an already-persisted config
would fail rather than update. Might be worth clarifying whether SET is
intended to be create-only, or whether the mutable-config case needs a
different path.
##########
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)
Review Comment:
🤖 Here (and in applyPropertyUnset at line 198) hoodie.properties is mutated
before `catalog.alterTable`. If `catalog.alterTable` throws, hoodie.properties
has already been updated/deleted while the Spark catalog stays unchanged,
leaving the two out of sync with no rollback — and on UNSET the delete is
irreversible. Is there a reason to persist to hoodie.properties first rather
than after the catalog update succeeds, or is some compensating cleanup
expected on failure?
<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]
