fix(aws): apply column comments when syncing to Glue - #19488
Conversation
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR fixes AWSGlueCatalogSyncClient column/partition comment syncing, which had been a silent no-op since the AWS SDK v2 upgrade — SDK v2 model classes are immutable, so the old setComments built a Column and discarded it. The fix rebuilds the columns and, importantly, the enclosing StorageDescriptor so the UpdateTableRequest actually carries the comments, and reworks change detection to compare the original against the rebuilt descriptor. The immutability handling, unchanged-column short-circuit, and unknown-column preservation all look correct and are well covered by the new tests. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. Code looks clean overall — the withComments rename, immutability fix, and test coverage are all well-done; one minor doc phrasing note below.
cc @yihua
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19488 +/- ##
============================================
+ Coverage 76.97% 77.87% +0.90%
+ Complexity 33855 33153 -702
============================================
Files 2575 2527 -48
Lines 143378 139944 -3434
Branches 17573 17589 +16
============================================
- Hits 110362 108980 -1382
+ Misses 24755 23346 -1409
+ Partials 8261 7618 -643
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! This PR fixes AWS Glue column-comment syncing that had silently no-op'd since the SDK v2 upgrade, by rebuilding the immutable Column/StorageDescriptor objects instead of discarding the built result. The withComments logic (preserving unknown columns, clearing known-but-undoc'd ones) and the switch to comparing the rebuilt descriptor against the already-fetched one both trace out correctly, and the tests cover the key cases. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR fixes AWSGlueCatalogSyncClient so column and partition-column comments are actually applied when syncing to Glue by rebuilding the immutable SDK v2 Column list and StorageDescriptor instead of discarding the rebuilt column, and correcting the always-false no-op comparison. No issues flagged from this automated pass, a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Closes apache#19316. setComments built a Column carrying the comment and dropped the result, so updateTableComments never changed anything, always returned false, and no column or partition column comment reached Glue. Before the AWS SDK v2 upgrade (apache#9347) this called column.setComment(...) on the mutable v1 model, which worked; the v2 models are immutable, so the columns have to be rebuilt. Rebuilding the column list is not sufficient on its own: StorageDescriptor is immutable too, and the request was sending the original descriptor, so its columns would still have carried no comments. The descriptor is rebuilt from the updated columns and it is that descriptor which is sent. The rebuild is conditional. Rebuilding unconditionally is not equality-preserving: when a table's column list was never set, storageDescriptor.columns() is an SDK auto-construct list and hasColumns() is false, so toBuilder().columns(<empty list>) flips it to true and the descriptor compares unequal to itself. That reported a change and sent an updateTable that changed nothing, on every sync, since the fetched table comes back the same way each time. The descriptor is now reused unchanged when no column moved. A column the storage schema says nothing about is left untouched rather than cleared. The pre-SDK-v2 code cleared it, but only nominally - it built a Column and discarded it, so no comment was ever applied and nothing can depend on that behaviour. Clearing is also the riskier reading: storage field names keep the Avro schema's case while a catalog may hold them lowercased, and a name that failed to match would silently wipe a comment. This matches HMSDDLExecutor.applyFieldComments, added for the Hive side in apache#19289. The change detection also compared a freshly fetched table against the local objects it had not modified, which was trivially equal and cost two extra Glue GetTable calls per sync. It now compares the already-fetched table against the rebuilt values, so one GetTable serves the whole method. Tests drive both withComments directly and updateTableComments end to end. Reaching the latter needed the fixture to produce a readable table schema: GlueTestUtil wrote its commit as JSON into .hoodie, but this is a table-version-8+ table whose active timeline lives under .hoodie/timeline and is read through CommitMetadataSerDe, so the instant was not on the timeline at all. The commit is now written through HoodieTestTable, which is what the rest of the repo uses, with the two test-jar dependencies it needs declared exactly as the sibling hudi-gcp and hudi-azure modules declare them. GlueTestUtil's hand-rolled createMetaFile is dead and goes with it. Each half of the fix is pinned by a test that fails without it: rebuilding the columns but sending the original StorageDescriptor fails only the end-to-end test, and restoring the unconditional rebuild fails only the no-columns case. hudi-aws: 102 tests, 0 failures. checkstyle 0, apache-rat 0.
1b6c3e9 to
27826f0
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 fixes column/partition comments not being applied when syncing to Glue after the AWS SDK v2 upgrade, by rebuilding the immutable Column list and StorageDescriptor instead of discarding the mutated copies. I traced the withComments branches, the no-op detection (including the SDK auto-construct empty-list case), and the rebuild path, and the logic holds up. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review. A few minor readability nits below, otherwise the fix and its tests are clear and well-documented.
cc @yihua
| */ | ||
| @Test | ||
| void testRebuildingColumnsRequiresRebuildingTheStorageDescriptor() { | ||
| Column column = GlueTestUtil.getColumn("name", "string", null); |
There was a problem hiding this comment.
🤖 nit: this test asserts UnsupportedOperationException on a raw AWS SDK list rather than any code in AWSGlueCatalogSyncClient — it's documenting SDK behaviour more than exercising Hudi logic. Could you fold the immutability assertion into testWithCommentsAppliesTheStorageComment as a comment, and rename this test to focus on what the Hudi code actually does (e.g. testWithCommentsReturnsANewDescriptorWhenColumnsChange)?
There was a problem hiding this comment.
Thanks - the first assertion is fair, but I would rather keep the test as it is.
Only the assertThrows is SDK characterisation. The other four assertions all run Hudi code: the test calls AWSGlueCatalogSyncClient.withComments, then checks that the original descriptor is untouched, that the rebuilt one carries the comment, and that original and updated compare unequal.
That last one is load bearing rather than incidental. updateTableComments decides whether to call updateTable purely by comparing the original descriptor against the rebuilt one, so descriptor equality is the Hudi logic here. It is also the assertion that would have caught the auto-construct-list bug fixed in this revision, where a rebuilt descriptor compared unequal to itself for a table whose column list was never set.
Folding the immutability check into testWithCommentsAppliesTheStorageComment would also mix two subjects: that test covers withComments on a plain list, this one covers the descriptor. Happy to reconsider if a committer prefers the split.
4 participants
Describe the issue this Pull Request addresses
Closes #19316.
AWSGlueCatalogSyncClient.updateTableCommentshas applied no column or partition column comments since theAWS SDK v2 upgrade (#9347). Its helper built a
Columncarrying the comment and threw the result away:Before the upgrade this called
column.setComment(...)on the mutable v1 model, which worked. SDK v2 modelclasses are immutable, so nothing was applied:
updateTableCommentsnever detected a change, alwaysreturned
false, and withhoodie.datasource.hive_sync.sync_comment=trueno comment ever reached Glue.Found while reviewing #19289, which fixed the equivalent Hive metastore paths.
Summary and Changelog
setCommentsbecomeswithComments, which returns a rebuilt list instead of mutating in place.The storage descriptor is rebuilt too. Rebuilding only the column list is not enough and is the part
worth reviewing:
StorageDescriptoris immutable as well, and theUpdateTableRequestwas sending theoriginal descriptor. Editing a copy of
storageDescriptor.columns()— which is what the issue textoriginally suggested — would still have shipped columns with no comments. The request now sends the
descriptor rebuilt from the updated columns.
A column the storage schema says nothing about is left untouched rather than cleared. The pre-SDK-v2
code cleared it, but only nominally: it built a
Columnand discarded it, so no comment was ever appliedand nothing can depend on that behaviour. Clearing is also the riskier reading:
getStorageFieldSchemaskeeps the Avro schema's case while a catalog may holdcolumn names lowercased, so a name that failed to match would silently wipe a user's comment. Columns the
schema does know are still authoritative — a known column with no doc has its comment cleared. This
matches
HMSDDLExecutor.applyFieldComments, added for the Hive side in fix(hive-sync): sync column and partition column comments to HMS #19289, so the two catalogs nowagree.
The descriptor rebuild is conditional. Found while re-reviewing before squashing: rebuilding
unconditionally is not equality-preserving. When a table's column list was never set,
storageDescriptor.columns()is an SDK auto-construct list andhasColumns()isfalse, sotoBuilder().columns(<empty list>)flips it totrueand the descriptor compares unequal to itself.updateTableCommentsthen reported a change and sent anupdateTablethat changed nothing, on everysync, because the fetched table comes back the same way each time. The descriptor is now reused unchanged
when no column moved.
partitionKeysis unaffected: it is compared with plainList.equals, where anauto-construct list and an empty list are equal.
Change detection now uses the table already fetched. It compared a freshly fetched table against local
objects it had not modified — trivially equal, and two extra Glue
GetTablecalls per sync. It nowcompares the fetched table against the rebuilt values, so one
GetTablecall does the job.Verification
withCommentsis@VisibleForTestingand covered by four new tests inTestAWSGlueSyncClient:testWithCommentsAppliesTheStorageCommenttestWithCommentsClearsTheCommentOfAKnownColumnWithoutADoctestWithCommentsLeavesColumnsTheStorageSchemaDoesNotKnowAlonetestRebuildingColumnsRequiresRebuildingTheStorageDescriptorstorageDescriptor.columns()is unmodifiable, and a descriptor rebuilt with new columns is a different object — the trap the original bug fell intoRestoring the build-and-drop behaviour inside
withCommentsturns three of them red, so they are notpassing vacuously:
Coverage gap now closed.
codecovreported 58.33% patch coverage with 5 uncovered lines, all insideupdateTableComments— the method this PR is about. The tests drovewithCommentsdirectly becauseupdateTableCommentswas unreachable from this module: it callsgetTableDoc(), which resolves the tableschema, and the fixture had none.
The cause was that
GlueTestUtilwrote its commit as JSON into.hoodie, while this is a table-version-8+table whose active timeline lives under
.hoodie/timelineand is read throughCommitMetadataSerDe. So theinstant was not on the timeline at all (
getActiveTimeline()returned[]), and putting it in the right placeby hand still failed, because
CommitMetadataSerDeexposes onlydeserialize.The fixture now writes the commit through
HoodieTestTable, as the rest of the repo does, which needs twotest-jar dependencies:
hudi-hadoop-commonforHoodieTestTableandhudi-commonfor theFileCreateUtilsit delegates to. Both are declared exactly as the sibling
hudi-gcpandhudi-azuremodules declare them —27 modules already depend on the
hudi-hadoop-commontest-jar — and there is no dependency cycle.GlueTestUtil's hand-rolledcreateMetaFileis dead as a result and is removed.Two tests now drive
updateTableCommentsend to end:testUpdateTableCommentsAppliesThemToColumnsAndPartitionKeysUpdateTableRequestcarries the comments on both the storage descriptor's columns and the partition keys, and the method reports a changetestUpdateTableCommentsIsANoOpWhenNothingChangesupdateTablecall at alltestUpdateTableCommentsIsANoOpWhenTheTableHasNoColumnsThe end-to-end test earns its keep on the half the unit tests could not reach. Rebuilding the column list
but sending the original
StorageDescriptor— the mistake the issue's own suggested fix would have made —fails only this new test and nothing else in the suite:
The no-columns case is pinned the same way. Restoring the unconditional
toBuilder().columns(...).build()fails only that test:The three commits on this branch are squashed into one, with a message describing the change as it now
stands rather than the sequence it was arrived at.
Impact
With
hoodie.datasource.hive_sync.sync_comment=true, Glue column and partition column comments start beingapplied on the update path, which is the documented behaviour and what worked before the SDK v2 upgrade.
Users who already have comments in Glue keep them: only columns the storage schema knows are touched.
Two extra
GetTablecalls per comment sync are removed. No API, config or table format change.Risk Level
low — one helper rewritten and its result actually used, in a path that currently does nothing at all.
The semantics of the "unknown column" case are deliberately narrower than the pre-SDK-v2 code; that is
argued above rather than hidden.
Documentation Update
none — no new config, and this restores documented behaviour rather than changing it.
Contributor's checklist