voonhous commented on code in PR #20188:
URL: https://github.com/apache/hudi/pull/20188#discussion_r4184563578
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/record/BaseRecordIndexer.java:
##########
@@ -392,19 +393,22 @@ public static <T> HoodieData<HoodieRecord>
convertMetadataToRecordIndexRecords(H
}
try {
- Map<String, List<HoodieWriteStat>> writeStatsByFileId =
allWriteStats.stream().collect(Collectors.groupingBy(HoodieWriteStat::getFileId));
- int parallelism = Math.max(Math.min(writeStatsByFileId.size(),
metadataConfig.getRecordIndexMaxParallelism()), 1);
+ // a file group is identified by its partition and file id, the file id
alone is only unique within a partition for files
+ // written outside Hudi
+ Map<HoodieFileGroupId, List<HoodieWriteStat>> writeStatsByFileGroupId =
allWriteStats.stream()
+ .collect(Collectors.groupingBy(writeStat -> new
HoodieFileGroupId(writeStat.getPartitionPath(), writeStat.getFileId())));
+ int parallelism = Math.max(Math.min(writeStatsByFileGroupId.size(),
metadataConfig.getRecordIndexMaxParallelism()), 1);
Review Comment:
Bew test does not cover the record-index change.
The test `assertRecordndex` will pass with or without the fix here. On
master, the record-index base-file branch already processed each write stat
with its own partition.
As of now, the test is failing because secondary-index throws, not because
of record index.
Can we add a unit case on `convertMetadataToRecordIndexRecords`?
I've asked Claude on how to do it and it says we can do two hand-built
`HoodieDeltaWriteStat` that share a file id accross p1 and p2. Then assert each
record's partition.
Not sure if there's a better way to do this.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/index/record/BaseRecordIndexer.java:
##########
Review Comment:
NIT: `logFileWriteStats.get(0).getPartitionPath()` ->
`writeStatsByFileGroupIdEntry.getKey().getPartitionPath()` ?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/SecondaryIndexRecordGenerationUtils.java:
##########
@@ -263,7 +267,7 @@ private static <T> HoodieData<HoodieRecord>
convertReplacedFileGroupsToSecondary
HoodieSchema tableSchema) {
List<Pair<String, String>> replacedFileGroups =
replaceCommitMetadata.getPartitionToReplaceFileIds().entrySet().stream()
.flatMap(partitionAndFileIds -> partitionAndFileIds.getValue().stream()
- .filter(fileId -> !writtenFileIds.contains(fileId))
+ .filter(fileId -> !writtenFileGroupIds.contains(new
HoodieFileGroupId(partitionAndFileIds.getKey(), fileId)))
Review Comment:
No test that covers replaced file group filter changes.
Can we add a second commit to the new test that replaces `brazil` and writes
a `part-00000.parquet` in a third partition, then assert that `bob` is gone
from the 2ndary index.
--
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]
