perf(common): avoid redundant V1 archived timeline loads in incremental queries - #19553
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19553 +/- ##
============================================
+ Coverage 76.84% 78.12% +1.27%
- Complexity 32379 33685 +1306
============================================
Files 2522 2540 +18
Lines 139106 141471 +2365
Branches 16713 18045 +1332
============================================
+ Hits 106892 110520 +3628
+ Misses 24621 23248 -1373
- Partials 7593 7703 +110
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the contribution! This PR splits IncrementalQueryAnalyzer.analyze into dedicated V1 and V2 paths so that V1 resolves incremental ranges by applying InstantRange directly to requestedTime and loads the filtered archived timeline at most once, avoiding the redundant second archive scan. I traced the new V1 branches against the V2 reference semantics in CompletionTimeQueryViewV2.getInstantTimes and the shared createQueryContext, and the behavior appears preserved. No correctness issues found. A few style/readability suggestions in the inline comments. Please take a look, and this should be ready for a Hudi committer or PMC member to take it from here. - a Hudi committer or PMC member can take it from here for a final review.
. A few naming and structural suggestions below, but overall the code is clean and well-organized.
cc @yihua
| } | ||
|
|
||
| @Test | ||
| void testV1ActiveOnlyAndSnapshotRangesDoNotLoadArchive() { |
There was a problem hiding this comment.
🤖 nit: this test covers two distinct cases (active-only range and earliest-snapshot range) in one method, which makes it harder to pinpoint which scenario caused a failure. Could you split it into testV1ActiveOnlyRangeDoesNotLoadArchive and testV1EarliestSnapshotDoesNotLoadArchive?
|
this is the potential improvement mentioned in #19338, please help review, thanks! @danny0405 |
cshuo
left a comment
cshuo
left a comment
There was a problem hiding this comment.
Two correctness issues in the V1 archived-timeline path.
| if (startCompletionTime.isEmpty() && endCompletionTime.isPresent()) { | ||
| // (_, end] returns the last eligible instant at or before end. Check the filtered active | ||
| // timeline first and only load the archive when there is no active match. | ||
| activeInstants = getLastInstantAtOrBefore(completedTimeline, endCompletionTime.get()); |
There was a problem hiding this comment.
This shortcut assumes that any eligible active instant is newer than every archived instant, which is not guaranteed with hoodie.archive.beyond.savepoint=true. A savepointed commit can remain active while later commits are archived; for example, active [101, 105] and archived [102, 103, 104]. For an end-only query ending at 104, this returns active instant 101 without checking the archive, omitting the actual latest eligible instant 104.
Please only use the active-only shortcut when the selected active instant is on or after the archive boundary; otherwise compare the best active and archived candidates. A regression test covering this savepoint-hole scenario would also be helpful.
| // streaming read speed limit, limits the maximum number of active commits allowed per run | ||
| activeInstants = activeInstants.subList(0, limit); | ||
| } | ||
| List<String> instants = Stream.concat(archivedInstants.stream(), activeInstants.stream()) |
There was a problem hiding this comment.
This concatenation does not guarantee global requested-time ordering. With hoodie.archive.beyond.savepoint=true, an old savepointed commit may remain active while later commits are archived. For example, archived [102, 103, 104] and active [101] produces [102, 103, 104, 101]. Since lastInstant is taken from the final list element, the resulting end boundary regresses to 101, and downstream readers can omit 102–104.
Please globally deduplicate and sort the combined requested-time list before deriving lastInstant. The previous V1 candidate path explicitly applied both distinct() and sorted(), so preserving those properties would also handle overlap during concurrent archival.
5 participants
Describe the issue this Pull Request addresses
This is a follow-up to #19338 and addresses the archived-timeline optimization suggested in #19338 (comment).
For timeline layout V1,
CompletionTimeQueryViewV1loaded archived instants to identify candidate requested times, after whichIncrementalQueryAnalyzerdiscarded that timeline and loaded the archive again to constructQueryContext. Since V1 completion time is equivalent to requested time, the second scan is redundant.Summary and Changelog
IncrementalQueryAnalyzer.InstantRangedirectly toHoodieInstant.requestedTime()and reuse the same archived timeline inQueryContext.Impact
Improves incremental-query performance for timeline layout V1 (table versions 5–7) by eliminating a redundant archived-timeline scan. There are no public API, configuration, storage-format, or V2 semantic changes. Query results and existing range semantics remain unchanged.
Risk Level
low
The change touches V1 incremental range selection but keeps V2 on the existing path. It is covered by
TestIncrementalQueryAnalyzerandTestCompletionTimeQueryViewV1: 16 tests passed with no failures or errors. Reactor compilation, Checkstyle, RAT, andgit diff --checkalso passed.Documentation Update
none. This is an internal performance optimization with no new user-facing behavior or configuration.
Contributor's checklist