[feature](query-cache) Support incremental merge for stale query cache entries#65482
Open
asdf2014 wants to merge 10 commits into
Open
[feature](query-cache) Support incremental merge for stale query cache entries#65482asdf2014 wants to merge 10 commits into
asdf2014 wants to merge 10 commits into
Conversation
…e entries Hourly batch loads keep bumping the version of the hot partition, so its query cache entries never hit again and every "last N days" aggregation recomputes the whole partition each hour. This change lets BE reuse a stale entry instead of discarding it: scan only the delta rowsets in (cached_version, current_version], emit their partial aggregation state side by side with the cached partial blocks (the upstream merge aggregation combines both), and write the merged entry back under the new version. Correctness rests on two properties: append-only snapshots decompose as S(v2) = S(v1) + delta, and partial aggregation states merge homomorphically. Every precondition guards one of them, and any violation falls back to a full recompute silently: - FE only sets TQueryCacheParam.allow_incremental (new optional field 8) when the experimental session switch enable_query_cache_incremental is on, the cache point is a non-finalize aggregation directly above the olap scan node, and the selected index is append-only: DUP_KEYS, or merge-on-write UNIQUE_KEYS. - BE re-validates per tablet at decision time: local storage mode, version order, the compaction threshold (query_cache_max_incremental_merge_count, BE config, default 8), keys type, delta capturability, no delete predicates in the delta, and for merge-on-write tables a delete-bitmap window check that rejects any load that rewrote pre-existing keys, so an occasional backfill costs exactly one full recompute and re-bases the entry. A new fragment-level QueryCacheRuntime makes one idempotent decision (HIT / INCREMENTAL / MISS) per instance, pins the entry and pre-captures the delta read source. Centralizing the decision also fixes three pre-existing defects: the scan/cache-source double-lookup race that could write back an empty poisoned entry, row-binlog scans polluting the cache, and build_cache_key failures aborting the whole query instead of degrading to uncached execution. Observability: profile fields HitCacheStale, IncrementalDeltaVersions and IncrementalFallbackReason, plus three BE metrics (query_cache_stale_hit_total, query_cache_incremental_fallback_total, query_cache_write_back_total). Benchmark (80M-row duplicate-key table, 200k-row hourly appends, group by a non-distribution column): a stale query drops from ~307ms (full recompute) to ~19ms with incremental merge, on par with an exact hit, and the cost no longer grows with the base data size.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
|
run buildall |
Contributor
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found three issues, all in the test/coverage layer for the query-cache incremental merge change. The BE/FE/storage implementation paths I reviewed did not reveal a substantiated data-correctness issue, but the current tests do not yet prove the actual stale incremental SQL path and one BE fixture masks setup failures.
Critical checkpoint conclusions:
- Goal/test proof: incomplete; the SQL regression does not assert that Mode::INCREMENTAL is reached.
- Scope/focus: the implementation is focused on query-cache incremental merge.
- Concurrency/lifecycle: shared cache decision, pinned cache handles, and pipeline local-state setup were reviewed; no reportable issue found.
- FE-BE/thrift compatibility: the optional field is appended and old/new mixed behavior falls back safely.
- Storage/version correctness: delta rowset capture, MOW delete bitmap checks, compaction fallback, and cloud fallback were reviewed; no reportable issue found.
- Testing standards: issues found in the regression and BE fixture, reported inline.
Validation: static review only; I did not run builds or tests in this review-only runner.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29834 ms |
Contributor
TPC-DS: Total hot run time: 180384 ms |
Contributor
ClickBench: Total hot run time: 25.73 s |
…tests Prove the incremental path in the regression through BE metrics: the first append round on the duplicate-key and the merge-on-write table must increase query_cache_stale_hit_total (the hot partition holds two data rowsets at that point, so neither the merge-count threshold nor a background compaction can interfere), and the delete-predicate and MoW backfill phases must each increase query_cache_incremental_fallback_total. Both counters only move when enable_query_cache_incremental is on (default off, this suite is the only one that sets it), and the assertions are one-sided deltas summed across all backends, so concurrent suites cannot break them. Cloud mode keeps the plain consistency checks since it always falls back by design. Align the suite with the regression standards: hardcoded table names, order_qt snapshots at each stable phase backed by a generated .out, and tables left in place after the run for debugging. Stop discarding setup statuses in the BE UT fixture: fatal assertions in SetUp() that print the failed Status, non-fatal checks with an early return in create_tablet() before the tablet is registered into the tablet map, null checks at the dereferencing call sites, and assertions on the two previously discarded booleans in init_rs_meta().
github-actions
Bot
removed
the
approved
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
|
Nice Job, I will review BE part code |
HappenLee
reviewed
Jul 14, 2026
| } | ||
| // Cloud tablets capture rowsets through a different (partly asynchronous) | ||
| // path; incremental merge only supports local storage for now. | ||
| if (config::is_cloud_mode()) { |
Contributor
There was a problem hiding this comment.
why the cloud mode not support now？if we sync_rowset we can also check the right status of the tablet, we should do sync_rowset async before do the check
Member
Author
There was a problem hiding this comment.
Cloud mode is planned. The first version sticks to local storage because the approach is easiest to validate there (the delta capture and the merge-on-write bitmap check run against a view that is complete by construction), which keeps the impact of this PR contained. Once the approach is confirmed workable, I will submit a separate follow-up PR to complete cloud support.
Contributor
There was a problem hiding this comment.
Very high level of completeness. The quality of the code output and the thoroughness of the comments are above the original code quality of Doris. Looking forward to more contributions from you.
HappenLee
reviewed
Jul 14, 2026
| // blocks for a write back that would be discarded anyway. Such an entry | ||
| // stays stale until compaction merges its delta away (then the capture | ||
| // above fails and a full recompute takes over). | ||
| if (decision->handle.get_cache_total_bytes() > _param.entry_max_bytes || |
Contributor
There was a problem hiding this comment.
Under what circumstances would this condition be true? Is it possible that the row count is set to 10 by the user during caching, and then set to 5 by the user on the second run?
Member
Author
There was a problem hiding this comment.
Exactly, that is the scenario. entry_max_bytes and entry_max_rows come from the session variables and are sent with every query, but they do not participate in the cache digest, so a query with a smaller limit still hits the entry written under a larger one. The write-back side already refuses to store anything above the current limits (the size accounting in cache_source_operator.cpp), so with unchanged limits a cached entry can never exceed them; the only way this condition turns true is a limit lowered after the entry was written: set in the same session, a different session using a smaller value for the same digest, or an admin lowering the global value to cap the cache memory footprint.
Without this branch such a query would still deep-copy the cached blocks into the write-back accumulation and pay one extra merge copy per delta block, only for the size accounting to clear it all once the running total crosses the limits; and since the entry can never be replaced under the new limits, copying up to the configured limit would repeat on every stale query. With the branch we skip the accumulation upfront but keep the delta scan benefit. Both directions are covered in the UTs (write_back_feasible true and false).
HappenLee
reviewed
Jul 14, 2026
| case TPlanNodeType::OLAP_SCAN_NODE: { | ||
| std::shared_ptr<QueryCacheRuntime> query_cache_runtime; | ||
| if (enable_query_cache) { | ||
| if (_query_cache_runtime == nullptr) { |
Contributor
There was a problem hiding this comment.
cache operator should create the _query_cache_runtime before, if here is nullptr, should return error or throw exception or dcheck here?
Member
Author
There was a problem hiding this comment.
Fixed in ba09543. The plan tree is built in pre-order and the cache source sits above the scan, so the runtime created with it must already exist when the scan node is reached; a missing one means FE sent a malformed plan shape. Silently creating a runtime here could even drop data: with a HIT decision the scan skips scanning while no cache source emits the entry. So the lazy creation is replaced with an InternalError. The same reasoning applies to the cache source side, whose old degrade-to-pass-through branch is now an error too, and both paths are covered by unit tests (QueryCacheFragmentContextTest on the scan side, QueryCacheOperatorTest.test_missing_runtime_fails_init on the cache source side).
HappenLee
reviewed
Jul 14, 2026
| pool, tnode, next_operator_id(), descs, _num_instances, | ||
| enable_query_cache ? _params.fragment.query_cache_param : TQueryCacheParam {}); | ||
| enable_query_cache ? _params.fragment.query_cache_param : TQueryCacheParam {}, | ||
| std::move(query_cache_runtime)); |
Contributor
There was a problem hiding this comment.
why use _query_cache_runtime just like cache operator, no need a query_cache_runtime
Member
Author
There was a problem hiding this comment.
The scan does need the shared runtime: the runtime is the single decision point for the instance. Whichever operator initializes first triggers get_or_make_decision(), and both consume the same decision object, so the pair always agrees: the scan skips scanning if and only if the cache source emits just the cached blocks (HIT), and scans only the delta if and only if the cache source merges it (INCREMENTAL). Two independent lookups are exactly the pre-existing race this PR removes: they could interleave with an eviction and write back an empty poisoned entry. The local shared_ptr copy was indeed unnecessary; ba09543 passes the member directly.
HappenLee
reviewed
Jul 14, 2026
HappenLee
reviewed
Jul 14, 2026
| const bool cache_incremental = | ||
| _query_cache_decision != nullptr && | ||
| _query_cache_decision->mode == QueryCacheInstanceDecision::Mode::INCREMENTAL; | ||
| const int64_t cache_start_version = |
Contributor
There was a problem hiding this comment.
This piece of code is useless. Later, the line auto delta_source = _query_cache_decision->take_delta_read_source(_tablets[i].tablet->tablet_id()) already ensures that the read version is correct. Instead, this code introduces unnecessary complexity and unless i think should removed the logic.
Member
Author
There was a problem hiding this comment.
Removed in ba09543 together with the plumbing below. The cache_incremental flag itself stays: it still gates take_delta_read_source() and keeps the parallel scanner builder off for delta scans (details in the sibling thread).
…version plumbing Address the second round of review comments: - replace the silent lazy creation at the scan node and the degrade-to-pass-through branch at the cache source with InternalError: a missing runtime means a malformed plan shape, and silently continuing could drop data on a HIT decision - drop OlapScanner::Params::start_version and its plumbing: the pre-captured delta read source already determines what gets read, and version.first has no behavioral consumer on the BE read path - pass the fragment's QueryCacheRuntime member directly instead of a local copy - cover both fail-loud paths with unit tests
Contributor
|
run buildall |
Contributor
|
/review |
Contributor
TPC-DS: Total hot run time: 176682 ms |
Contributor
ClickBench: Total hot run time: 25.51 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
…napshot cached blocks zero-copy Address the third review round on the incremental merge: Entry commit: the write-back entry was published from an unconditional Defer that also ran during error unwinding. On the final delta block eos is observed BEFORE the block is merged, so a merge failure there (allocation failure surfaces as a non-OK Status through RETURN_IF_CATCH_EXCEPTION) published an entry that was missing the failed block under the current version, and a later exact hit would serve the incomplete result silently. The commit now happens only at the two explicit success exits (the empty pull that observes eos, and the exit where the pulled block was fully consumed), so every error return leaves the cache untouched and the pinned stale entry stays the served truth. This also closes the exception-unwind exit (the old Defer ran the insert inside a destructor while unwinding, which could std::terminate if the insert itself threw), and the insert is latched exactly-once, so even an out-of-contract extra poll after eos cannot replace the committed entry with an empty one. Write-back copies: replacing an entry peaked at roughly three payloads (the pinned old entry, the deep-copied write-back set, and the insert-time copy), and the LRU accounting could not see the pinned one once the same-key insert displaced it. The cached blocks are now snapshotted as zero-copy views sharing the COW column pointers of the pinned entry, and the single materialization stays in QueryCache::insert, so a replacement peaks at the old entry plus the new entry. Delta blocks were already moved and are unchanged. Tests: a new case injects a deterministic merge failure on the final delta block and asserts the query fails, nothing is published at the current version, and the stale entry is untouched; the reordered write-back case now pins the column sharing before insert, the single materialization after insert, and that an extra poll after eos keeps the committed entry intact; test_no_hit_cache1 asserts the latched flag, with the element count as the insert witness.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review completed; requesting changes for one performance issue.
- Goal, scope, and clarity: the PR adds an opt-in stale query-cache path that combines a pinned cached partial-aggregation result with an exactly captured tablet-version suffix. The FE eligibility checks, BE storage revalidation, and conservative full-recompute fallbacks accomplish that goal for the reviewed correctness paths, and the change remains focused on query-cache planning, execution, storage validation, observability, and tests.
- Concurrency and locks: scan/cache-source local states can initialize concurrently, but they publish one immutable fragment decision under a narrow mutex and transfer each tablet read source once under a separate take lock. Tablet header/delete-bitmap locking and the existing queue synchronization have consistent scopes; heavy capture and cache work remain outside the fragment critical section. I found no deadlock or unsafe atomic/lifecycle signal.
- Lifecycle, error handling, and memory safety: cache handles and rowset readers pin their data through consumption, COW block views remain immutable until the single tracked cache materialization, and replacement occurs exactly once only after successful queue EOS and merge/accounting. Error, cancellation, limit, and failed-merge paths retain the old entry. No ownership cycle, abnormal release, or cross-translation-unit static-initialization dependency was found.
- Configuration and compatibility: the merge-count threshold is mutable and read for each decision; the experimental session switch follows the existing forwarded-variable path. The new Thrift field is optional and BE checks
__isset, preserving mixed FE/BE behavior. Cloud, binlog, force-refresh, unsupported key modes, delete predicates, and unsafe MOW-history paths conservatively fall back. - Parallel and conditional paths: exact hit, miss, incremental, publication-race, DUP, MOW, one-/two-phase aggregation, finalized/nested/distinct, selected-index, and replacement paths were traced. The remaining issue is the unconditional incremental-scan exception to
ParallelScannerBuilder: the code assumes a small delta without enforcing any size bound. - Tests and results: FE, BE runtime/operator/fragment, and end-to-end regression coverage exercise enablement, safe stale hits, merge-count rebasing, races, one-shot ownership, slot permutation, write-back/error behavior, delete predicates, and MOW history fallback. The generated aggregate results were checked against the SQL. No test covers a long-dormant cache entry followed by a large suffix, which is the requested coverage for the inline issue.
- Observability: stale-hit, fallback, and write-back counters are registered and updated on winning paths, while profiles retain hit state, delta window, fallback reason, and insertion state. No additional observability defect was found.
- Persistence, transactions, and data writes: this changes only the process-local read cache; it adds no storage format, EditLog, durable transaction, or table-write path. Failed refreshes leave the prior cache entry intact.
- FE-BE propagation: the eligibility bit is serialized in the query-cache parameter on the fragment path and is guarded for older senders; the session control is forwarded through the existing annotated mechanism. No missing parallel sending path was found.
- Performance: MM-1 must be addressed because a dormant entry can accumulate an unbounded suffix while still bypassing intra-tablet rowset/segment parallelism.
- User focus: no additional review focus was supplied; the whole PR was reviewed.
Validation was static only, as required by this review runner; no build or test command was executed here.
Contributor
TPC-H: Total hot run time: 29611 ms |
Contributor
TPC-DS: Total hot run time: 177382 ms |
Contributor
ClickBench: Total hot run time: 25.56 s |
… deltas Address the fourth review round on the incremental merge: The incremental scan bypassed ParallelScannerBuilder on the assumption that deltas are small by construction, but nothing enforces that bound: query_cache_max_incremental_merge_count limits prior write-backs, not the versions accumulated while an entry sits idle, so the first stale query after a long dormant stretch can face an arbitrarily large suffix (including cumulative-compaction rowsets wholly inside the window) and was scanning it with one scanner per tablet. The bypass is removed: the builder consumes the captured delta read sources exactly as it consumes full captures (each partial read source keeps the entire source's delete predicates and delete bitmap, and the rowset readers are cloned per scanner), so a large suffix is split by rowset/segment rows like any full scan, while a genuinely small delta still collapses to a single scanner through min_rows_per_scanner, which is the size gate the old bypass was approximating from outside. Tests: the regression suite gains a dormant-entry phase: fill the entry, land thirty loads on a single-tablet partition without touching it, then assert the first stale query still takes the incremental path and agrees with the uncached result, with parallel_scan_min_rows_per_scanner lowered so the builder really splits the delta into many scanners.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
There was a problem hiding this comment.
I found one new issue: the new incremental session switch is forwarded to a master FE without forwarding its required enable_query_cache prerequisite, so forwarded statements silently skip query-cache normalization. The inline comment contains the concrete call path and requested fix.
Apart from that integration gap, the local-storage design is coherent: eligibility is limited to mergeable partial aggregates over supported key models, stale-entry decisions pin a complete (cached,current] snapshot, merge-on-write history rewrites force fallback, and replacement entries commit only after successful eos. I did not repeat the dormant parallel-scanner coverage gap because existing discussion discussion_r3599901507 already tracks that exact point.
Key checkpoint conclusions:
- Goal and proof: the core incremental-hit/fallback paths are implemented and broadly covered by FE, BE, fragment-context, and regression additions, but the forwarding defect prevents the user-facing goal from being complete in a multi-FE forwarded-statement path. The supplied tests were reviewed statically, not executed under this review contract.
- Scope and focus: the 22-file patch is focused on FE eligibility/protocol propagation, BE snapshot/runtime/cache-source behavior, metrics/configuration, and corresponding tests. No extra user focus was supplied.
- Concurrency and lifecycle: decision publication, take-once delta-source transfer, cache-handle/rowset pinning, queue eos ordering, cancellation/error behavior, and exact-once successful writeback were traced; no additional race, deadlock, leak, or initialization-order issue survived review.
- Configuration: the new dynamic delta-load limit has a clear zero-disable behavior and is read for new decisions; no unsafe runtime-update behavior was found.
- Compatibility: the optional Thrift flag preserves rolling FE/BE compatibility, unsupported cloud/binlog/storage cases fall back, and no persisted metadata format changes. Session forwarding is the one incomplete compatibility boundary called out inline.
- Parallel and fallback paths: HIT, INCREMENTAL, MISS, full scan, merge failure, compaction/path loss, delete predicates, MoW rewrites, cloud, and row-binlog paths were checked. The dormant regression's claimed multi-scanner exercise remains a known existing-thread test gap rather than a duplicate new comment.
- Conditions and explanations: incremental authorization and storage rejection conditions are documented consistently with the implementation; future expansions must preserve the upstream merge and bitmap-version invariants.
- Tests: additions cover FE authorization, BE decisions/operators/fragment wiring, exact/incremental/fallback results, metrics, and several negative paths. A follower-to-master forwarding test is missing, and the accepted comment requests it.
- Observability: hit/incremental/miss/fallback metrics and runtime-profile counters expose the new decisions without an additional issue found.
- Transactions and persistence: the feature mutates only ephemeral query-cache state; cache replacement is delayed until successful completion and leaves the old entry untouched on error/cancellation.
- FE/BE contract:
allow_incrementalis optional and defaults safely on older senders/receivers; the dependent FE session-variable forwarding contract is incomplete as reported. - Performance: delta capture avoids holding the fragment decision lock during storage work and scans bitmap version groups; no new unsupported full-history hot-path issue was found. The previously discussed parallel-test weakness remains unresolved coverage, not a second inline finding.
Validation was static-only as required by the review bundle; no builds or tests were run. Reviewed head: 2033145942b320146968faa57bcff45814dc260b.
…lanning master Address the fifth review round on the incremental merge: A forwarded statement is planned by the master in a fresh ConnectContext, which starts from the master's global values and then sees only what getForwardVariables() sends, and query cache normalization runs wherever the statement is planned. Only enable_query_cache_incremental was annotated needForward, so a client that enabled both switches on a follower reached the master with the incremental flag true while enable_query_cache fell back to the master's global (false by default): both planner gates then skipped cache normalization and the feature silently did nothing. The base switch is now forwarded too, which also fixes the same gap the base query cache had on its own before this PR: a session-level setting, or a SET_VAR hint, never reached the planner of a forwarded statement. Forwarding the switch alone would have left a second gap one hop away: QueryCacheNormalizer builds the cache param on the planning FE from three more session variables, so with the cache now engaged on forwarded paths a forced refresh would have been dropped and the entries would have been sized by the master's defaults. Those three (query_cache_force_refresh, query_cache_entry_max_bytes, query_cache_entry_max_rows) are forwarded as well, so every variable the query cache reads at plan time now travels with the statement. The digest is unaffected: it is salted from the variables annotated as affecting the query result, which needForward does not join. Behavior change to note for multi-FE clusters: a forwarded statement now plans with the session's query cache state instead of the master's global one. Statements are forwarded when the follower cannot read, when force_forward_all_queries or the debug point is on, and for every redirected statement, which includes each non-group-commit INSERT issued on a follower. Besides the cache itself, enable_query_cache carries the scan-side semantics it already has locally: OlapScanNode pins the fixed replica and, as upstream documents on skip_missing_version, that pinning turns the missing-version rescue off. Those now apply to forwarded statements too, exactly as they already do when the same session runs the same statement without forwarding, so the forwarded path stops being the odd one out. Three honesty fixes found while reviewing the above: The delete bitmap scan claimed the entries it reads are immutable once their version is visible, hence race free. They are not: compaction copies markers onto its output keeping the original version, and the stale-rowset aggregation re-stamps a swept range onto its end version, both at already visible versions and both on the local path. The check is still safe, but for a different reason, now written down: it is conservative (an in-window marker on a rowset outside the delta is read as a rewrite), the capture endpoint check rejects a window whose path was merged away, and what is left is the narrow non-atomic window between capture and scan that the exact-hit path shares. query_cache_write_back_total counts entries handed to the cache, not entries the cache kept: with an LRU-K admission a first-seen key on a full shard is dropped, so on a pressured BE the counter can climb while nothing lands. The metric text says so now. The dormant-entry regression phase never split its delta: the per-scanner floor defaults to 2M rows, so the 210-row delta stayed with one scanner and the phase covered none of the splitting it was written for. It now lowers the floor to the reachable minimum of 1024 rows (BE clamps the variable up to that) and grows the delta to 6000 rows across 30 rowsets, which the builder cuts into four or five scanners. Tests: SessionVariablesTest covers the forwarding contract end to end, setting all five variables on a follower-side session, sending them through getForwardVariables() and asserting a fresh master-side session receives every one of them instead of its defaults.
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: one P1 wrong-result race remains in the merge-on-write incremental-cache decision.
Critical checkpoint conclusions
- Goal and proof: The FE-to-BE flow selects an eligible non-final partial aggregation, carries the optional incremental flag, captures delta rowsets, merges cached and delta aggregate states, and writes the refreshed entry. The changed FE, BE, and regression tests exercise authorization, stale hits, designed fallbacks, forwarding, write-back, and result equivalence. The append-only path is supported, but the MoW path is not correct under the compaction/GC interleaving in the inline finding, and no test covers it.
- Scope and focus: The 23 changed files are focused on incremental-cache eligibility, decision/runtime plumbing, pipeline integration, configuration/metrics, and tests. The implementation appropriately reuses the existing query-cache and scan pipelines, but its correctness depends on storage bitmap lifetime that the change does not preserve.
- Concurrency and locks: Fragment decision publication moves metadata work outside the fragment map mutex, and I found no reverse lock-order or deadlock issue in the new path. The blocker is the non-atomic sequence between version-path capture and live delete-bitmap inspection: compaction, stale sweeping, and unused-rowset GC can remove cached-side evidence because delta capture pins only delta rowsets.
- Lifecycle and memory: Cache handles, captured read sources, queue dependencies, COW block ownership, entry-size rejection, error/eos/limit exits, and exactly-once write-back were reviewed; no cycle, static-initialization, or additional release issue was found. The unresolved lifecycle defect is specifically the cached-side delete marker described inline.
- Configuration and compatibility: The mutable merge-count threshold is read at decision time and non-positive values conservatively force full recomputation. The new Thrift field is optional and guarded by
__isset; all planner-read session variables are forwarded, old FE/BE peers degrade safely, cloud and binlog paths are disabled, and there is no storage-format or EditLog change. - Parallel paths and conditions: DUP_KEYS is append-only for this purpose. MoR UNIQUE, AGG, cloud, invalid-key, and binlog paths fall back; scan-range identity and per-instance source consumption are consistent. Local MoW is the only accepted correctness issue. No analogous missed change was found in the other guarded paths.
- Tests and results: I inspected the FE/BE unit tests, regression suite, and generated result file, including negative/fallback and dormant-delta cases. They were not executed because this review environment explicitly prohibits builds and test runs. A deterministic sync-point test for capture followed by boundary-crossing compaction, immediate stale sweep, and unused-rowset GC is still required.
- Observability and performance: The new hit/fallback/write-back metrics and diagnostic reasons cover the critical outcomes; the bounded bitmap-group walk and out-of-lock decision build address the previously raised performance concerns. No additional observability or performance blocker was found.
- Transactions, persistence, and data writes: The feature writes only ephemeral cache state and changes no Doris table data, transaction protocol, persisted metadata schema, or replay path. The accepted issue is nevertheless a read-snapshot correctness failure that can publish a wrong cache entry.
- Other review checks: Status/error handling, ownership, version bounds, nullable handling, and deterministic test-output conventions were checked where applicable. After deduplication against all existing threads and three convergence rounds, no other valuable finding remains.
Contributor
FE UT Coverage ReportIncrement line coverage |
…rite delta Address the P1 on the incremental merge decision. The delta capture pins only the delta rowsets, so on a merge-on-write table the rewrite markers classification looks for have no other owner. Those markers live on the rowsets the cached snapshot was built from, and a compaction spanning the cached and delta versions retires such a rowset without relocating an overwritten row's marker: the row has no counterpart in the compaction output, so calc_compaction_output_rowset_delete_bitmap skips it and the marker stays on the retired input. The stale sweep then hands that input to the unused-rowset GC, which drops its delete bitmap. If that lands between the delta capture and the bitmap scan, _delta_rewrites_history sees no rewrite, the cached aggregate is merged with the rows that replaced it, and a wrong entry is stored at the current version and served by later exact hits. The classification now pins the cached side first: under the tablet header lock it captures the rowsets of [0, cached_version] and holds them across _delta_rewrites_history. start_delete_unused_rowset() collects a retired rowset only once nothing else references it, and only a collected rowset reaches remove_rowset_delete_bitmap(), so the reference kept here keeps the evidence too. A quiet capture returns the prefix it walked, so the pinned path is checked to cover the whole [0, cached_version] range; a short prefix (its own history compacted away and swept) falls back to a full recompute rather than classifying against evidence it cannot vouch for. Tests: two cases against a real storage engine. One pins the cached side across a merge-on-write classification and asserts, through a sync point, that the full [0, cached_version] range is held at that moment; the other gives the cached side a hole so the quiet capture returns a short prefix and asserts the coverage check falls back. Both fail on the pre-fix code (the first because the pin is not held, the second because the incremental path is taken instead of the fallback).
Member
Author
|
run buildall |
Member
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29821 ms |
Contributor
TPC-DS: Total hot run time: 177006 ms |
Contributor
ClickBench: Total hot run time: 25.62 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Related PR: N/A
Problem Summary:
For hourly batch-load workloads, every load bumps the tablet version of the
hot partition, so its query cache entries never hit again: each "last N days"
aggregation recomputes the whole hot partition every hour, and BE CPU spikes
during load windows.
This PR adds incremental merge for the query cache (experimental, default
off). When a cache entry's version falls behind, BE reuses it instead of
discarding it: it scans only the delta rowsets in
(cached_version, current_version], emits their partial aggregation stateside by side with the cached partial blocks so the existing upstream merge
aggregation combines both, and writes the merged entry back under the new
version. The execution plan is unchanged.
Measured impact (80M-row duplicate-key table, 200k-row hourly appends,
group by a non-distribution column): a stale query drops from ~307 ms
(full recompute) to ~19 ms with incremental merge, about 16x and on
par with an exact hit (~17 ms); the cost no longer grows with the base data
size (8M rows: 48 ms full vs 19 ms incremental; 80M rows: 307 ms vs 19 ms).
Full numbers in the Verification section below.
Design highlights:
S(v2) = S(v1) ⊎ Δfor append-only data plus thehomomorphism
partial(A ⊎ B) ≡ merge(partial(A), partial(B)). Everyprecondition guards one of the two properties; any violation falls back
to a full recompute silently, so results are always correct.
(
TQueryCacheParam.allow_incremental) only for non-finalize aggregationsdirectly above the olap scan on an append-only index: DUP_KEYS, or
merge-on-write UNIQUE_KEYS. Merge-on-read UNIQUE and AGG tables always
fall back.
in the profile): cloud mode, version order, the compaction threshold
(
query_cache_max_incremental_merge_count, default 8, 0 disables), keystype, delta capturability on the version graph, delete predicates in the
delta, and for merge-on-write tables a delete-bitmap window check that
rejects loads rewriting pre-existing keys. A rare backfill therefore costs
exactly one full recompute, which re-bases the entry, and the next
pure-append load is incremental again.
QueryCacheRuntimemakes one idempotent decision perinstance (HIT / INCREMENTAL / MISS), pins the entry and pre-captures the
delta read source. This also fixes three pre-existing defects: the
scan/cache-source double-lookup race (could write back an empty poisoned
entry), row-binlog scans polluting the cache, and
build_cache_keyfailures aborting the query instead of degrading to uncached execution.
after
query_cache_max_incremental_merge_countmerges the next queryrecomputes in full and compacts the entry; oversized entries keep the
delta-scan benefit but skip the write back.
Verification:
FE UT:
QueryCacheNormalizerTest13/13 (8 assertions on the incrementalauthorization matrix: switch, plan shapes, DUP / MoW / MoR / AGG / nested
aggregation).
BE UT: 34/34 across the decision layer, the incremental scenarios (MoW
pure-append / history-rewrite / irrelevant bitmap entries, version gap,
capture error via debug point, delete predicates) and the operator layer.
llvm-cov: zero uncovered changed lines;
query_cache.cppat 100% linecoverage.
Regression:
query_cache_incrementalpasses on a real 1FE+1BE cluster;every step is checked against a cache-off baseline. BE metrics confirm the
incremental path actually fires (
stale_hit_total+40 across the suite,fallbacks exactly at the three designed spots).
Benchmark (80M-row DUP table, 200k-row hourly appends, group by a
non-distribution column, 5 rounds each):
A stale query becomes as cheap as an exact hit, and its cost no longer
grows with the base data size (8M rows: 48ms full vs 19ms incremental;
80M rows: 307ms vs 19ms).
Release note
Add experimental incremental merge for the query cache: a stale entry can be
reused by scanning only the delta rowsets and merging them with the cached
partial aggregation state. Controlled by the session variable
enable_query_cache_incremental(default off) and the BE configquery_cache_max_incremental_merge_count(default 8).Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)