hudi-agent commented on code in PR #20273:
URL: https://github.com/apache/hudi/pull/20273#discussion_r4245772280
##########
hudi-common/src/main/java/org/apache/hudi/core/read/BaseHoodieTableFileIndex.java:
##########
@@ -95,6 +96,10 @@
@Slf4j
public abstract class BaseHoodieTableFileIndex implements AutoCloseable {
+ private static final Set<String> WRITE_AND_SAVEPOINT_ACTIONS =
CollectionUtils.createSet(
Review Comment:
🤖 nit: `WRITE_AND_SAVEPOINT_ACTIONS` is defined identically here and in
`CompletionTimeQueryViewV2`; would it make sense to define it once (e.g. in
`HoodieTimeline`) and reuse it?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -102,14 +128,18 @@ public CompletionTimeQueryViewV2(HoodieTableMetaClient
metaClient) {
*/
public boolean isCompleted(String instantTime) {
// archival does not proceed beyond the first savepoint, so any instant
before that is completed.
- return this.instantTimeToCompletionTimeMap.containsKey(instantTime) ||
isArchived(instantTime);
+ return this.instantTimeToCompletionTimeMap.containsKey(instantTime)
+ || InstantComparison.compareTimestamps(instantTime, LESSER_THAN,
this.firstNonSavepointCommit);
}
/**
* Returns whether the instant is archived.
*/
public boolean isArchived(String instantTime) {
- return InstantComparison.compareTimestamps(instantTime, LESSER_THAN,
this.firstNonSavepointCommit);
+ // with archival beyond savepoint, the write instants between a
savepointed commit and the first active write
+ // instant are archived as well, while the savepointed commit itself stays
active.
+ return InstantComparison.compareTimestamps(instantTime, LESSER_THAN,
this.firstActiveWriteInstant)
+ && !this.activeSavepointedInstants.contains(instantTime);
Review Comment:
🤖 non-blocking: `isArchived` can now go false then true along the timeline
(savepointed C1 is not archived, but C2–C6 after it are), while
`IncrementalQueryAnalyzer.splitInstantByActiveness` still treats everything
from the first non-archived instant onward as active.
So with a range that starts before C1's completion, or a hole that has a
second savepointed commit inside it, C2–C6 land in the active list. They are
not on the active timeline, so they are silently dropped. The full-scan
fallback doesn't catch this, because it filters on `includedCommits`.
Could the split go instant by instant instead, e.g. `filter(isArchived)` /
`filter(!isArchived)`, with the merged instants sorted by requested time? A
window starting at `1000` in `testArchivedInstantsAfterSavepointedCommit` would
cover this.
<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]
