hudi-agent commented on code in PR #19804:
URL: https://github.com/apache/hudi/pull/19804#discussion_r3900954979
##########
hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordScanner.java:
##########
@@ -220,9 +230,44 @@ protected AbstractHoodieLogRecordScanner(HoodieStorage
storage, String basePath,
}
this.partitionNameOverrideOpt = partitionNameOverride;
+ this.deletePartitionPathOpt = resolveDeletePartitionPath(basePath,
logFilePaths, partitionNameOverride);
this.recordType = recordMerger.getRecordType();
}
+ private static Option<String> resolveDeletePartitionPath(String basePath,
List<String> logFilePaths,
+ Option<String>
partitionNameOverride) {
+ if (partitionNameOverride.isPresent()) {
+ return partitionNameOverride;
+ }
+ if (logFilePaths.isEmpty()) {
+ return Option.empty();
+ }
+ return Option.of(getRelativePartitionPath(
Review Comment:
π€ This changes where the delete's partition path comes from: previously each
`DeleteRecord.getPartitionPath()` was used per-record, now it's derived once
from `partitionNameOverride` or the first log file's parent dir and applied to
every delete in the block. For these single-file-group scanners that should
always match, but I want to double-check β is there any path (e.g. a legacy
delete block whose `DeleteRecord`s carry a partition different from the
physical log location) where the stored partition could now differ? @yihua does
relying on the log file's physical partition for deletes hold across all read
paths here?
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/log/AbstractHoodieLogRecordScanner.java:
##########
@@ -220,9 +230,44 @@ protected AbstractHoodieLogRecordScanner(HoodieStorage
storage, String basePath,
}
this.partitionNameOverrideOpt = partitionNameOverride;
+ this.deletePartitionPathOpt = resolveDeletePartitionPath(basePath,
logFilePaths, partitionNameOverride);
this.recordType = recordMerger.getRecordType();
}
+ private static Option<String> resolveDeletePartitionPath(String basePath,
List<String> logFilePaths,
+ Option<String>
partitionNameOverride) {
+ if (partitionNameOverride.isPresent()) {
+ return partitionNameOverride;
+ }
+ if (logFilePaths.isEmpty()) {
+ return Option.empty();
+ }
+ return Option.of(getRelativePartitionPath(
+ new StoragePath(basePath), new
StoragePath(logFilePaths.get(0)).getParent()));
Review Comment:
π€ nit: could you add a brief inline comment here explaining why `get(0)` is
safe β something like `// all log files in a file group share the same parent
partition directory`? Without it a future reader might wonder whether this
silently breaks for multi-directory log paths.
<sub><i>β οΈ AI-generated; verify before applying. React π/π to flag
quality.</i></sub>
--
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]
