fix(sync): drop the duplicate UpdateTable in updateTableSchema - #19762
fix(sync): drop the duplicate UpdateTable in updateTableSchema#19762niranjan-1408 wants to merge 4 commits into
Conversation
…e schema changes The cascade rebuilt each partition's path from its values as KEY=VALUE via getStringFromPartition, then turned that string back into a StorageDescriptor location. On a table with hive_style_partitioning=false, which is the default, the on-disk layout is bare values, so an ALTER COLUMN TYPE rewrote every partition's location to a prefix holding no data. Correct values meant Glue matched the existing partition and performed an update rather than rejecting an unknown one, so the catalog silently disagreed with storage and Hive metastore readers returned no rows for the affected partitions. The cascade now reuses the location Glue already holds and the values it already returned, changing only the columns, which is correct for every key generator and encoding because it never derives a path. updatePartitionsToTableInternal and the cascade share one implementation; partitions are passed lazily so a failure deriving them from a storage path is still wrapped rather than escaping unwrapped.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! This PR removes a redundant second awsGlue.updateTable(request) call in updateTableSchema that fired unconditionally and cost an extra Glue table version per schema change. I traced the logic: the request object is immutable and unchanged between the two calls, and the cascade block only updates partitions (via batchUpdatePartition) while re-reading the table to source columns — so keeping the first call (before the cascade) preserves ordering, and the removed second call was a pure no-op version bump. The added test correctly exercises the non-cascade path and asserts exactly one UpdateTable. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Both callers now share updatePartitionsInternal, so the only thing separating them is where the location comes from. The cascade half is asserted; this pins the other half, which composes the sync base path with the storage-relative path.
| assertEquals(partitionLocation, entries.get(0).partitionInput().storageDescriptor().location()); | ||
| assertEquals(Collections.singletonList("2024-01-15"), entries.get(0).partitionValueList()); | ||
| assertEquals(table.storageDescriptor().columns(), | ||
| entries.get(0).partitionInput().storageDescriptor().columns()); |
There was a problem hiding this comment.
[P2] Please make this assertion verify the updated columns, not the pre-update table schema. getSimpleSchema() contains both id and name, but the getTable stub always returns table, whose descriptor contains only name; consequently the cascade currently propagates that stale list and this test explicitly accepts it. This misses the ordering invariant used to justify keeping the first UpdateTable call. Could the first getTable return the original table and the cascade re-read return a table built from the captured update request (or otherwise containing the new columns), then assert those new columns here? An InOrder check for updateTable before batchUpdatePartition would cover the same dependency.
The getTable stub returned the same pre-update table on every call, so the cascade propagated a stale column list and the assertion compared it against itself. The second read now carries the new columns and the assertion checks those, and an InOrder check pins updateTable before batchUpdatePartition, which is the ordering the cascade depends on to source columns.
updateTableSchema issued the same UpdateTable request object twice. The second call sat outside the if (cascade) block, so it fired unconditionally, including on non-partitioned tables where cascade is false. Nothing between the two calls mutates the request, and the cascade branch writes partitions rather than the table, so the second call was a no-op that Glue still counted as a new table version. Every schema change therefore consumed two table versions where one would do. The first call is kept rather than the second because the cascade re-reads the table to source the columns it propagates: with the update first it sees the new columns, whereas keeping only the second would have it propagate stale ones.
fdc1775 to
c81929c
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 removes a redundant second awsGlue.updateTable(request).get() call in updateTableSchema that fired the same immutable request twice, doubling Glue table-version churn per schema change. I traced the flow and confirmed keeping the first call is correct — the cascade branch (updatePartitionsToTable) re-reads the table so it must run after the schema write, and the second update carried identical content, making it a true no-op. No issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
cc @yihua
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19762 +/- ##
============================================
- Coverage 77.97% 73.08% -4.90%
+ Complexity 33466 31407 -2059
============================================
Files 2539 2539
Lines 140955 140979 +24
Branches 17013 17018 +5
============================================
- Hits 109911 103030 -6881
- Misses 23386 30695 +7309
+ Partials 7658 7254 -404
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
5 participants
Stacked on #19761, please review that one first. This PR shows both commits until it merges.
Describe the issue this Pull Request addresses
updateTableSchemaissues the sameUpdateTablerequest object twice. The second call sits outside theif (cascade)block, so it fires unconditionally, including on non-partitioned tables. Nothing between the two calls mutates the request and the cascade branch writes partitions rather than the table, so the second call is a no-op that Glue still counts as a new table version. Every schema change therefore consumes two table versions where one would do.Summary and Changelog
Removes the second
awsGlue.updateTable(request).get(). The first is kept rather than the second because the cascade re-reads the table to source the columns it propagates: with the update first it sees the new columns, whereas keeping only the second would have it propagate stale ones. Adds a test asserting exactly oneUpdateTableper schema change.Impact
Halves the Glue table-version churn from this method. No behaviour change beyond the removed redundant write.
Risk Level
low
One deleted line. The added test fails against unfixed code with
TooManyActualInvocations: Wanted 1 time but was 2.Documentation Update
none
Contributor's checklist