fix: ALTER TABLE Hudi property persistence - #19733
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Spark SQL ALTER TABLE ... SET/UNSET TBLPROPERTIES handling for Hudi V2 tables so that Hudi-related table configs are persisted to (or removed from) the table’s .hoodie/hoodie.properties, instead of being catalog-only.
Changes:
- Persist Hudi table-config keys to
hoodie.propertiesonALTER TABLE ... SET TBLPROPERTIES. - Delete Hudi table-config keys from
hoodie.propertiesonALTER TABLE ... UNSET TBLPROPERTIES. - Keep non-Hudi properties as Spark-catalog-only, matching Spark’s default behavior.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| HoodieWriterUtils.validateTableConfig( | ||
| sparkSession, | ||
| tableConfigs, | ||
| metaClient.getTableConfig) | ||
|
|
There was a problem hiding this 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.
| 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) | ||
| } |
There was a problem hiding this 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.
| def applyPropertyUnset(sparkSession: SparkSession): Unit = { | ||
| val catalog = sparkSession.sessionState.catalog | ||
| val propKeys = changes.map(_.asInstanceOf[RemoveProperty]).map(_.property()) | ||
| // ignore NonExist unset | ||
| propKeys.foreach { k => | ||
| if (!table.properties.contains(k) && k != TableCatalog.PROP_COMMENT) { | ||
| logWarning(s"Cannot remove property [$k] because it is not currently set for the table.") | ||
| } | ||
| } | ||
| val tableComment = if (propKeys.contains(TableCatalog.PROP_COMMENT)) None else table.comment | ||
| val newProperties = table.properties.filter { case (k, _) => !propKeys.contains(k) } | ||
| val newTable = table.copy(properties = newProperties, comment = tableComment) | ||
| deleteHoodieTableConfigs(sparkSession, propKeys) | ||
| catalog.alterTable(newTable) | ||
| logInfo("table properties change finished") | ||
| } | ||
|
|
||
| // to do support set default value to columns, and apply them to internalSchema | ||
| def applyPropertySet(sparkSession: SparkSession): Unit = { | ||
| val catalog = sparkSession.sessionState.catalog | ||
| val properties = changes.map(_.asInstanceOf[SetProperty]).map(f => f.property -> f.value).toMap | ||
| // This overrides old properties and update the comment parameter of CatalogTable | ||
| // with the newly added/modified comment since CatalogTable also holds comment as its | ||
| // direct property. | ||
| 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") |
voonhous
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19733 +/- ##
============================================
+ Coverage 77.95% 77.97% +0.02%
- Complexity 33447 33467 +20
============================================
Files 2539 2539
Lines 140891 141027 +136
Branches 17008 17052 +44
============================================
+ Hits 109829 109965 +136
+ Misses 23401 23399 -2
- Partials 7661 7663 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR persists Hudi table properties to hoodie.properties on ALTER TABLE SET/UNSET TBLPROPERTIES, reusing the existing option-mapping and table-config validation. A couple of correctness edge cases are worth double-checking in the inline comments, particularly which keys get written into hoodie.properties and the ordering relative to the Spark catalog update. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. One small readability nit on a non-obvious idiom in the delete path.
| */ | ||
| private def updateHoodieTableConfigs(sparkSession: SparkSession, properties: Map[String, String]): Unit = { | ||
| val tableConfigs = HoodieOptionConfig.mapSqlOptionsToTableConfigs( | ||
| HoodieOptionConfig.extractHoodieOptions(properties)) |
There was a problem hiding this 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?
| val newTable = table.copy( | ||
| properties = table.properties ++ properties, | ||
| comment = properties.get(TableCatalog.PROP_COMMENT).orElse(table.comment)) | ||
| updateHoodieTableConfigs(sparkSession, properties) |
There was a problem hiding this 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?
| } | ||
| private def deleteHoodieTableConfigs(sparkSession: SparkSession, propertyKeys: Seq[String]): Unit = { | ||
| val tableConfigs = HoodieOptionConfig.mapSqlOptionsToTableConfigs( | ||
| HoodieOptionConfig.extractHoodieOptions(propertyKeys.map(_ -> "").toMap)) |
There was a problem hiding this 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.
| HoodieWriterUtils.validateTableConfig( | ||
| sparkSession, | ||
| tableConfigs, | ||
| metaClient.getTableConfig) | ||
|
|
There was a problem hiding this 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.
| 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) | ||
| } |
There was a problem hiding this 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.
| if (tableConfigs.nonEmpty) { | ||
| val metaClient = getMetaClient(sparkSession) | ||
|
|
||
| HoodieWriterUtils.validateTableConfig( |
There was a problem hiding this comment.
[P1] Validate invariants against the merged table config
validateTableConfig mainly compares each incoming key with an existing value; it does not re-run the cross-property validation used at table creation. For example, a normal single-partition table can already have hoodie.datasource.write.hive_style_partitioning=true and no explicit slash-separated property. Setting hoodie.datasource.write.slash.separated.date.partitioning=true passes here because the new key has no existing value and the partition count is one, then persists both flags even though HoodieCatalogTable.extraTableConfig explicitly rejects that pair as mutually exclusive. Please validate the merged existing+incoming configuration, including cross-property and value constraints, before updating hoodie.properties.
| val tableComment = if (propKeys.contains(TableCatalog.PROP_COMMENT)) None else table.comment | ||
| val newProperties = table.properties.filter { case (k, _) => !propKeys.contains(k) } | ||
| val newTable = table.copy(properties = newProperties, comment = tableComment) | ||
| deleteHoodieTableConfigs(sparkSession, propKeys) |
There was a problem hiding this comment.
[P1] Normalize aliases on both sides before deleting
The catalog removal above uses the requested spelling verbatim, but this call canonicalizes SQL aliases before deleting from hoodie.properties. If the catalog contains the supported legacy alias preCombineField and the user unsets orderingFields, newProperties leaves preCombineField in the catalog while this path maps orderingFields to hoodie.table.ordering.fields and deletes the persisted config. That recreates the catalog/file divergence this PR is meant to fix. Please resolve aliases to one canonical identity when deciding what to remove from both stores.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the updates here — the new revision meaningfully tightens the validation. Walking through the prior feedback:
- Immutable-config protection (my prior BUG + Copilot's two comments + danny0405 P1): Addressed for the immutable case. The new
protectedTableConfigsset + explicit checks now reject SET that would change an immutable table config and UNSET that would remove one, and the overly-strictvalidateTableConfig(which rejected re-setting any existing key) is gone. The new test covers SET-new, SET-changed, Spark-only property, UNSET mutable, UNSET Spark property, and the immutable reject paths. Good coverage. - Broad
hoodie.*persistence (the other half of my prior BUG): Still present by design — non-table mutable keys likehoodie.keep.max.commits/hoodie.clean.commits.retainedare written intohoodie.properties(tests 1–2 assert this intentionally). That now reads as a deliberate choice rather than an oversight, so I won't re-block on it; a committer may still want to confirm that persisting arbitrary write/engine configs as table-level defaults is the intended semantics. - Ordering/atomicity between
hoodie.propertiesand the Spark catalog (my prior ARCHITECTURE finding): Not touched in this revision, so it remains open. One improvement as a side effect: validation now throws beforeHoodieTableConfig.update/delete, so pure reject cases no longer leave partial state — but the success-path cross-store consistency (hoodie.properties mutated vs. catalog) is unchanged. - danny0405's alias-normalization-before-delete (line 199): Also not addressed in this revision — on UNSET, the catalog side uses the verbatim spelling while the hoodie.properties side canonicalizes aliases, so the two stores can diverge on which key was removed.
- Readability nit (
map(_ -> "").toMap): Unchanged; still a minor nit, not blocking.
No new correctness issues spotted in the added validation logic. Please take a look at the still-open ordering/alias items above (already tracked in existing threads), and this should be ready for a Hudi committer or PMC member to take it from here.
64cec58 to
2e0d5f7
Compare
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR persists Hudi table properties into hoodie.properties on ALTER TABLE SET/UNSET TBLPROPERTIES, with immutable-config guards. Traced the new update/delete paths (both merge/remove only the targeted keys, so no data-loss), and the remaining correctness concerns — the pre-catalog.alterTable mutation ordering, alias normalization on delete, and immutable-key protection — are already covered by inline comments from earlier rounds. No new critical issues flagged from this automated pass. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here. A couple of naming and doc-placement nits below.
cc @yihua
| * 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( |
There was a problem hiding this 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.
| logInfo("table properties change finished") | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this 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?
github-actions
Bot
added
size:M
| HoodieTableConfig.META_FIELDS_MODE.key, | ||
| HoodieTableConfig.BOOTSTRAP_BASE_PATH.key, | ||
| HoodieTableConfig.DATABASE_NAME.key, | ||
| HoodieTableConfig.TIMELINE_LAYOUT_VERSION.key, |
There was a problem hiding this comment.
[P1] Protect the physical timeline paths
This immutable-config set includes the timeline layout version but omits HoodieTableConfig.TIMELINE_PATH and TIMELINE_HISTORY_PATH. Both keys pass extractHoodieOptions, so setting hoodie.timeline.path to another directory rewrites hoodie.properties. Every newly constructed HoodieTableMetaClient derives its timeline location from that property, so it will point away from the existing instants and the table can appear to have no history; UNSET similarly breaks a table that legitimately uses a custom path by reverting it to the default. These structural paths need the same protection, or the implementation should use an explicit allowlist of mutable properties.
| HoodieTableConfig.DATABASE_NAME.key, | ||
| HoodieTableConfig.TIMELINE_LAYOUT_VERSION.key, | ||
| HoodieTableConfig.TABLE_STORAGE_LAYOUT.key, | ||
| HoodieTableConfig.TABLE_FORMAT.key |
There was a problem hiding this comment.
[P1] Keep metadata-partition state system-managed
HoodieTableConfig.TABLE_METADATA_PARTITIONS and TABLE_METADATA_PARTITIONS_INFLIGHT are also absent from this set. Since arbitrary hoodie.* keys reach HoodieTableConfig.update/delete, SQL can mark an unbuilt metadata partition as completed or remove a real completed/inflight marker. Readers and index support trust getMetadataPartitions to decide which metadata indexes are available, while the metadata writer uses both sets to decide what to update, so altering these values can route reads to missing or stale indexes and corrupt subsequent metadata maintenance. These internal state keys must not be user-mutable through TBLPROPERTIES.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the update! The only change since the last review is a new assertion in TestAlterTable.scala verifying that hoodie.clean.commits.retained is persisted as "10" in the table config after table creation — a reasonable strengthening of the test's baseline before the SET/UNSET assertions. Nothing new to flag from this automated pass.
On the open items: this test addition partially responds to Copilot's coverage concern (line 216) by asserting hoodie.properties state, but the substantive correctness threads remain open — Copilot/hudi-agent on the validateTableConfig rejecting re-SET of existing keys (line 237), the UNSET deleting configs without validation (line 296), danny0405's P1s on validating against the merged config (233), alias normalization on delete (199), and protecting the physical timeline paths / metadata-partition state in the immutable set (249/251). My two earlier readability nits (protectedTableConfigs naming, misplaced Scaladoc) are also still open. None of these are addressed by the test-only change here.
Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
Describe the issue this Pull Request addresses
Closes #19722
ALTER TABLE SET/UNSET TBLPROPERTIES for Hudi tables only updated the Spark catalog and did not persist the corresponding Hudi table properties in hoodie.properties.
Summary and Changelog
Persist Hudi table properties changed through ALTER TABLE SET/UNSET TBLPROPERTIES into hoodie.properties.
Changes:
Impact
Low user-facing impact. ALTER TABLE SET/UNSET TBLPROPERTIES now correctly persists and removes Hudi table configurations in hoodie.properties. Non-Hudi Spark table properties continue to behave as catalog-only properties.
No public API changes and no expected performance impact.
Risk Level
low
The change is limited to ALTER TABLE property handling and uses the existing Hudi table-config validation and persistence APIs. TestAlterTable was executed with 33/33 tests passing, and the Maven build completed successfully.
Documentation Update
none