hudi-agent commented on code in PR #19785:
URL: https://github.com/apache/hudi/pull/19785#discussion_r3948512540
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieFileGroup.java:
##########
@@ -160,7 +160,21 @@ private boolean isFileSliceCommitted(FileSlice slice) {
return false;
}
- return timeline.containsOrBeforeTimelineStarts(slice.getBaseInstantTime());
+ if (timeline.containsOrBeforeTimelineStarts(slice.getBaseInstantTime())) {
+ return true;
+ }
+
+ // An uncommitted base instant with a base file keeps the original
semantics: the slice stays
+ // hidden and is never revived by a committed log, otherwise the
uncommitted base file would be
+ // read in full (there is no read-side trimming for it before table
version 8).
+ if (slice.getBaseFile().isPresent()) {
+ return false;
+ }
+
+ // Log-only slice: safe to admit once a log is based off a committed
instant, the log reader
+ // filters out uncommitted log blocks.
Review Comment:
🤖 I don't think this rationale holds for v8+: both
`AbstractHoodieLogRecordScanner` (L331) and `BaseHoodieLogRecordReader` (L302)
only skip uncommitted blocks when `tableVersion.lesserThan(EIGHT)`. On v8+ —
the only layout where this clause fires — the guarantee comes from the view's
`filterUncommittedFiles`/`filterUncommittedLogs`, and APIs that don't trim
(`getAllFileSlices(partition)`, `getLatestUnCompactedFileSlices`,
`getAllFileGroups`) will now hand out the failed t2 log. Could you double-check
that every consumer you rely on goes through the trimming path, and update the
comment to reflect the actual invariant?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieFileGroup.java:
##########
@@ -160,7 +160,21 @@ private boolean isFileSliceCommitted(FileSlice slice) {
return false;
}
- return timeline.containsOrBeforeTimelineStarts(slice.getBaseInstantTime());
+ if (timeline.containsOrBeforeTimelineStarts(slice.getBaseInstantTime())) {
+ return true;
+ }
+
+ // An uncommitted base instant with a base file keeps the original
semantics: the slice stays
+ // hidden and is never revived by a committed log, otherwise the
uncommitted base file would be
+ // read in full (there is no read-side trimming for it before table
version 8).
+ if (slice.getBaseFile().isPresent()) {
Review Comment:
🤖 Does this guard leave the base-file flavour of the same NBCC bug in place?
Under NBCC the bucket fileId is deterministic (`newBucketFileIdForNBCC` =
bucketId + `CONSTANT_FILE_ID_SUFFIX`, used by Flink
`BucketBulkInsertWriterHelper` and Spark
`BaseSparkBucketIndexBucketInfoGetter`), so a failed bulk_insert base file at
t2 plus committed logs t3/t4 land in one slice that stays hidden here — yet for
v8+ `filterUncommittedFiles` already strips the uncommitted base file, so
admitting it would be safe. I realise the guard is there for pre-v8 (no
base-file trimming), but since this only fires on v8+ layouts anyway, is the
extra restriction intended?
<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]
