fix(hive-sync): make skip_ro_suffix take precedence over sync_snapshot_with_table_name - #19427
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19427 +/- ##
=========================================
Coverage 75.55% 75.56%
- Complexity 32657 32663 +6
=========================================
Files 2574 2574
Lines 142995 142999 +4
Branches 17530 17531 +1
=========================================
+ Hits 108038 108051 +13
+ Misses 26922 26911 -11
- Partials 8035 8037 +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! This PR fixes HiveSyncTool.doSync() so that when both skip_ro_suffix=true and sync_snapshot_with_table_name=true are set on a MERGE_ON_READ table, the redundant bare-name RT sync is skipped, keeping the bare table name registered as the read-optimized view. I traced the interaction with roTableName and the RO/RT sync ordering, and the guarded conditional correctly targets only the conflicting case while leaving all other config combinations unchanged. 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. One small wording nit in the warn log — "base table name" risks confusion with Hudi's "base file" concept.
cc @yihua
|
The single-round versus two-round difference behind my |
|
@wombatu-kun comments are addressed. |
| // a second sync round with a new commit: the bare table name's read-optimized sync (which runs | ||
| // immediately before the origin-table sync within the same doSync() call) advances its own | ||
| // last-commit-time-synced marker, so the origin-table step is the only one still eligible to run | ||
| // and flip the format if not guarded |
There was a problem hiding this comment.
This comment says the read-optimized sync advancing its own last-commit-time-synced marker is what leaves the origin-table step eligible, but that advance is exactly why a single round does not reproduce the flip. Not a blocker: what actually keeps the origin step eligible in the second round is HoodieHiveSyncClient serving a per-run cached table snapshot taken before that write, so either name that or drop the explanation.
There was a problem hiding this comment.
Dropped the explanation.
…t_with_table_name hoodie.datasource.hive_sync.skip_ro_suffix=true claims the bare table name for the read-optimized (RO) view of a MOR table. Hudi 1.x flipped the default of hoodie.meta.sync.sync_snapshot_with_table_name from false to true (HUDI-7415), so under the default ALL hive-sync-table-strategy, HiveSyncTool.doSync() now syncs the bare table name twice in the same run when both configs are true: once as the RO table, then again as the RT table, with the RT sync silently winning. This makes skip_ro_suffix a no-op and breaks read-optimized queries against the bare table name in case the engine can't read with HoodieParquetRealtimeInputFormat (e.g., Presto). This change skips the redundant bare-name RT sync when skip_ro_suffix is set, and logs a WARN naming the table and stating which config wins. The real-time view remains available at <table>_rt.
…ndant comment, strengthen regression test to a second sync round
6bd8413 to
bce565b
Compare
|
Comments addressed. |
…t_with_table_name (#19427) * fix(hive-sync): make skip_ro_suffix take precedence over sync_snapshot_with_table_name hoodie.datasource.hive_sync.skip_ro_suffix=true claims the bare table name for the read-optimized (RO) view of a MOR table. Hudi 1.x flipped the default of hoodie.meta.sync.sync_snapshot_with_table_name from false to true (HUDI-7415), so under the default ALL hive-sync-table-strategy, HiveSyncTool.doSync() now syncs the bare table name twice in the same run when both configs are true: once as the RO table, then again as the RT table, with the RT sync silently winning. This makes skip_ro_suffix a no-op and breaks read-optimized queries against the bare table name in case the engine can't read with HoodieParquetRealtimeInputFormat (e.g., Presto). This change skips the redundant bare-name RT sync when skip_ro_suffix is set, and logs a WARN naming the table and stating which config wins. The real-time view remains available at <table>_rt. * Address review comments: fix bare-table-name wording, remove now-redundant comment, strengthen regression test to a second sync round * Drop the second-round explanation comment per review feedback (cherry picked from commit 8ee40b4)
…t_with_table_name (#19427) * fix(hive-sync): make skip_ro_suffix take precedence over sync_snapshot_with_table_name hoodie.datasource.hive_sync.skip_ro_suffix=true claims the bare table name for the read-optimized (RO) view of a MOR table. Hudi 1.x flipped the default of hoodie.meta.sync.sync_snapshot_with_table_name from false to true (HUDI-7415), so under the default ALL hive-sync-table-strategy, HiveSyncTool.doSync() now syncs the bare table name twice in the same run when both configs are true: once as the RO table, then again as the RT table, with the RT sync silently winning. This makes skip_ro_suffix a no-op and breaks read-optimized queries against the bare table name in case the engine can't read with HoodieParquetRealtimeInputFormat (e.g., Presto). This change skips the redundant bare-name RT sync when skip_ro_suffix is set, and logs a WARN naming the table and stating which config wins. The real-time view remains available at <table>_rt. * Address review comments: fix bare-table-name wording, remove now-redundant comment, strengthen regression test to a second sync round * Drop the second-round explanation comment per review feedback (cherry picked from commit 8ee40b4)
6 participants
Describe the issue this Pull Request addresses
Hudi 1.x flipped the default of
hoodie.meta.sync.sync_snapshot_with_table_namefromfalsetotrue([HUDI-7415]). Under the defaultALLhive-sync-table-strategy,HiveSyncTool.doSync()now syncs the bare table name of a MERGE_ON_READ table twice in the same run whenhoodie.datasource.hive_sync.skip_ro_suffix=trueis also set: once as the read-optimized (RO) table (sinceskip_ro_suffixredirects the RO sync onto the bare name instead of<table>_ro), then again as the real-time (RT) table for the "sync origin table" step, with the RT sync silently winning. This makesskip_ro_suffixa no-op, leaves no RO view registered anywhere, and breaks read-optimized queries against the bare table name for engines that can't readHoodieParquetRealtimeInputFormat(e.g. Presto/Trino).Summary and Changelog
Skips the redundant bare-name RT sync in
HiveSyncTool.doSync()whenskip_ro_suffixis set, logging a WARN naming the table and stating which config takes precedence. The real-time view remains available at<table>_rt; the bare table name stays registered as the RO view.No code was copied.
Impact
No public API change. Behavior change only for MERGE_ON_READ tables with both
hoodie.datasource.hive_sync.skip_ro_suffix=trueandhoodie.meta.sync.sync_snapshot_with_table_name=true: the bare table name now stays registered as the read-optimized view instead of being flipped to real-time.Risk Level
low
Change is scoped to a single conditional in
HiveSyncTool.doSync(); all other config combinations (including the previous default ofsync_snapshot_with_table_name=false) are unaffected. Covered by a new regression test that reproduces the pre-fix flip.Documentation Update
none
Contributor's checklist