[feature](query-cache) Support incremental merge for stale query cache entries#65482
[feature](query-cache) Support incremental merge for stale query cache entries#65482asdf2014 wants to merge 2 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.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
github-actions
Bot
added
the
approved
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
| // (including those where incremental merge falls back to a full recompute, | ||
| // e.g. cloud mode) while exercising the incremental path on local storage. | ||
| suite("query_cache_incremental") { | ||
| def tableName = "test_query_cache_incremental" |
There was a problem hiding this comment.
This new regression does not follow the local regression-test standards: ordinary single test tables should use hardcoded names instead of def tableName, determined result checks should be generated through qt/order_qt style expected output rather than only ad hoc assertEquals, and the tables should be dropped before use but left in place after the suite for debugging. Please align the suite with those standards while keeping any extra cache-on/cache-off behavioral checks you need.
There was a problem hiding this comment.
Aligned in 26484d7: table names are hardcoded now, five order_qt_* snapshots backed by a generated .out (via -forceGenOut against a real cluster) anchor the deterministic results at each phase, and the final DROP TABLE statements are removed so the tables stay in place for debugging. The cache-on/off double-run comparison is kept on top as the behavioral check.
| // Compare the cached query result against the uncached one, twice: the | ||
| // first cached run may fill or incrementally merge the entry, the second | ||
| // one should serve it. | ||
| def checkConsistency = { String sqlText -> |
There was a problem hiding this comment.
This helper only proves that the cache-enabled result matches the uncached result. The suite even documents that it passes when incremental merge falls back to a full recompute, so a regression where the real SQL path never takes Mode::INCREMENTAL would still pass. Please add an assertion that at least one post-insert local-storage query actually records a stale incremental hit, for example via HitCacheStale / IncrementalDeltaVersions in the profile or a monotonic increase in query_cache_stale_hit_total, and assert the intended fallback phase separately.
There was a problem hiding this comment.
Added in 26484d7. On local storage the suite now proves through BE metrics that the incremental path really fires: the first append round on both the duplicate-key table and the merge-on-write table must strictly increase query_cache_stale_hit_total. These rounds are chosen because the hot partition holds only two data rowsets at that point, so neither the merge-count threshold nor a background compaction can interfere. The two designed fallback phases (a delete predicate in the delta, and a MoW backfill that rewrites history) must each increase query_cache_incremental_fallback_total, so each intended fallback phase is asserted separately. Both counters only move when enable_query_cache_incremental is on (default off, and 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. Verified on a 1 FE + 1 BE cluster: the counters do move at all four asserted points.
| _engine = engine.get(); | ||
| ExecEnv::GetInstance()->set_storage_engine(std::move(engine)); | ||
| _data_dir = std::make_unique<DataDir>(*_engine, _absolute_dir); | ||
| static_cast<void>(_data_dir->init()); |
There was a problem hiding this comment.
Please do not discard these setup Status values. If _data_dir->init() fails, the fixture still proceeds to register tablets and the later incremental/fallback assertions can run against an invalid test environment. The same pattern appears below for tablet_meta->add_rs_meta(rs_meta) and tablet->init(). Check each status with a form valid for the enclosing function, for example a fatal assertion in SetUp() and either a non-fatal check plus early failure return, or a refactor to a void/fatal helper, inside create_tablet(), before using the constructed tablet state.
There was a problem hiding this comment.
Fixed in 26484d7: SetUp() now uses fatal assertions and prints the failed Status; create_tablet() checks add_rs_meta() and tablet->init() with a non-fatal check plus an early return nullptr before the tablet is registered into the tablet map, and the two call sites that dereference the result assert it is not null. The two discarded booleans in init_rs_meta() (JsonToProtoMessage, init_from_pb) are asserted as well. QueryCache* UTs pass 34/34 with the change.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29834 ms |
TPC-DS: Total hot run time: 180384 ms |
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
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
5 participants
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)