nsivabalan commented on code in PR #19045:
URL: https://github.com/apache/hudi/pull/19045#discussion_r3611571904
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -2081,6 +2192,14 @@ private SerializableFunction<HoodieRecord, HoodieRecord>
getRecordTagger(String
FileSlice slice = fileSlices.get(mappingFunction.apply(r.getRecordKey(),
fileGroupCount));
r.unseal();
r.setCurrentLocation(new
HoodieRecordLocation(slice.getBaseInstantTime(), slice.getFileId()));
+ // Under layouts that place file groups in sub-directories (e.g.
bucketing), the file slice's
+ // physical partition path may differ from the logical MDT partition
name carried on the
+ // record. Realign the record's partition path with where the file slice
actually lives so
+ // downstream HoodieAppendHandle's partition-path consistency check
passes.
+ String physical = slice.getPartitionPath();
Review Comment:
Acknowledged — this is the load-bearing correctness concern for the bucketed
layout and the current CI failure on `testMDTTableServicesWithBucketing` is
exactly this pattern. The writer-side realignment in `getRecordTagger` makes
incremental (delta-commit-driven) compaction/cleaning aware of the bucket, but
you are right that the full-listing fallbacks do not fan out:
- `BaseHoodieCompactionPlanGenerator.getPartitions()` returns logical MDT
partition names via `FSUtils.getAllPartitionPaths`, then calls
`fileSystemView.getLatestFileSlicesStateless(<logical>)` — under bucketing this
returns zero slices because the default `HoodieTableFileSystemView` lists only
the logical partition dir non-recursively.
- Cleaning’s `getPartitionPathsForFullCleaning()` has the same shape.
Fix direction (will land in a follow-up commit on this branch before merge,
tracking in `PR_FEEDBACK_TRIAGE.md`): make `HoodieTableFileSystemView`
layout-aware for MDT — when a logical MDT partition is requested, fan out
across `layout.getPhysicalPartitions(...)` sourced from the MDT-persisted
`hoodie.metadata.layout.partition.file.group.counts` property (no FS listing).
That closes the compactor and cleaner in one place. I will also flip the
failing test to assert directly on
`BaseHoodieCompactionPlanGenerator.generateCompactionPlan` producing ≥ 1
operation per bucket — asserting on the plan, not the side effect — so a future
regression fires immediately.
cshuo raised the same concern below (thread on
`HoodieTableMetadataUtil.java` line 1514) — converging both there.
##########
hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadataUtil.java:
##########
Review Comment:
Confirmed — this is the same class of bug as the writer-side thread on
`HoodieBackedTableMetadataWriter.java` above, and the CI failure on
`testMDTTableServicesWithBucketing` is exactly this pattern (compaction opens
`record_index/.record-index-0003-0_...log` at the flat path when the file is
really at `record_index/000003/`). Full details + fix direction in that thread
— the short version is: make `HoodieTableFileSystemView` layout-aware for MDT
so `getLatestFileSlicesStateless(<logical partition>)` fans out across
`layout.getPhysicalPartitions(...)`. That closes both compaction and the
cleaner’s full-listing path (`getPartitionPathsForFullCleaning`) in one place.
Landing as a follow-up commit on this branch before merge.
##########
hudi-common/src/main/java/org/apache/hudi/BaseHoodieTableFileIndex.java:
##########
@@ -349,15 +349,27 @@ private Map<PartitionPath, List<FileSlice>>
filterFiles(List<PartitionPath> part
// API. Note that for COW table, the merging logic of two slices does
not happen as there
// is no compaction, thus there is no performance impact.
HoodieTableFileSystemView finalFileSystemView = fileSystemView;
+ // For an MDT under a non-flat layout, the FS view indexes file slices
by their physical
+ // partition (bucket sub-paths), but the partition list here uses
logical partition names.
+ // Resolve via HoodieTableMetadataUtil which fans out across the
layout's physical sub-paths.
+ final boolean isMdt = HoodieTableMetadata.isMetadataTable(basePath);
return partitions.stream().collect(
Collectors.toMap(
Function.identity(),
- partitionPath ->
- queryInstant.map(instant ->
-
finalFileSystemView.getLatestMergedFileSlicesBeforeOrOn(partitionPath.path,
queryInstant.get())
- )
- .orElseGet(() ->
finalFileSystemView.getLatestFileSlices(partitionPath.path))
- .collect(Collectors.toList())
+ partitionPath -> {
+ if (isMdt) {
+ return queryInstant.isPresent()
Review Comment:
Good catch, and yes — the divergence is real for non-flat layouts. After
`e28eef5e345c` the flat default is back to the pre-PR path
(`getLatestMergedFileSlicesBeforeOrOn(queryInstant.get())`), so at least
existing tables are unaffected. But the non-flat branch still calls
`getPartitionLatestMergedFileSlices`, which internally uses "latest completed +
latest compaction" rather than `queryInstant`. That is a semantic change scoped
to bucketed tables and is not what the PR name promises.
Fix direction (follow-up commit on this branch, before merge): thread
`queryInstant` through a layout-aware merged-slice helper — same fan-out shape
as the compaction/cleaning fix in the thread on
`HoodieBackedTableMetadataWriter.java`, but with
`getLatestMergedFileSlicesBeforeOrOn(<physical partition>, queryInstant.get())`
under each physical sub-path and the results merged. That preserves time-travel
semantics for bucketed tables and keeps flat bit-identical.
--
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]
