wombatu-kun commented on code in PR #19427:
URL: https://github.com/apache/hudi/pull/19427#discussion_r3687612274
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java:
##########
@@ -211,7 +211,16 @@ protected void doSync() {
syncHoodieTable(snapshotTableName, true, false);
// sync origin table for MOR
if (config.getBoolean(META_SYNC_SNAPSHOT_WITH_TABLE_NAME)) {
- syncHoodieTable(tableName, true, false);
+ if
(config.getBoolean(HIVE_SKIP_RO_SUFFIX_FOR_READ_OPTIMIZED_TABLE)) {
+ // skip_ro_suffix explicitly claims the bare table name for
the RO view;
+ // an implicit sync_snapshot_with_table_name default must not
repurpose it as RT.
Review Comment:
The comment says an implicit `sync_snapshot_with_table_name` default must
not repurpose the name, but this guard drops that config however the user set
it - `HoodieSyncConfig` materializes the default into the props, so the
distinction is not visible here. Reword the comment to drop the
implicit-versus-explicit framing.
##########
hudi-sync/hudi-hive-sync/src/test/java/org/apache/hudi/hive/TestHiveSyncTool.java:
##########
@@ -1557,6 +1559,43 @@ public void testSyncMergeOnReadWithStrategy(String
syncMode, HoodieSyncTableStra
}
}
+ @Test
+ void testSkipRoSuffixTakesPrecedenceOverSnapshotWithTableName() throws
Exception {
+ // skip_ro_suffix explicitly claims the bare table name for the RO view;
the now-default-true
+ // sync_snapshot_with_table_name must not be allowed to flip it to RT.
+ hiveSyncProps.setProperty(HIVE_SYNC_TABLE_STRATEGY.key(),
HoodieSyncTableStrategy.ALL.name());
+
hiveSyncProps.setProperty(HIVE_SKIP_RO_SUFFIX_FOR_READ_OPTIMIZED_TABLE.key(),
"true");
+ hiveSyncProps.setProperty(META_SYNC_SNAPSHOT_WITH_TABLE_NAME.key(),
"true");
+ hiveSyncProps.setProperty(HIVE_SYNC_AS_DATA_SOURCE_TABLE.key(), "true");
+
+ String instantTime = "100";
+ String deltaCommitTime = "101";
+ HiveTestUtil.createMORTable(instantTime, deltaCommitTime, 5, true, true);
+
+ reInitHiveSyncClient();
+ reSyncHiveTable();
Review Comment:
Verified locally: with a fresh metastore and a single sync round the
bare-name RT step is already skipped by the `isAlreadySynced` guard in
`syncHoodieTable`, so this test passes on master with the production hunk
reverted. Adding a second commit (`HiveTestUtil.addMORPartitions`) plus another
`reSyncHiveTable()` round makes it fail as intended, with the bare table left
on `HoodieParquetRealtimeInputFormat`.
##########
hudi-sync/hudi-hive-sync/src/main/java/org/apache/hudi/hive/HiveSyncTool.java:
##########
@@ -211,7 +211,16 @@ protected void doSync() {
syncHoodieTable(snapshotTableName, true, false);
// sync origin table for MOR
if (config.getBoolean(META_SYNC_SNAPSHOT_WITH_TABLE_NAME)) {
- syncHoodieTable(tableName, true, false);
+ if
(config.getBoolean(HIVE_SKIP_RO_SUFFIX_FOR_READ_OPTIMIZED_TABLE)) {
Review Comment:
This looks like the fix for the still-open regression in #16637 (HUDI-8264,
priority:high). Adding a `Fixes #16637` reference to the description would
close it on merge.
--
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]
