Repository navigation
Unify PromQL instant and range query fetch/merge paths #581
Description
Activity
is_exact_querystill hardcoded false for range queries — #580 not fully fixedfinish_range_context(asap-query-engine/src/engines/simple_engine/promql.rs:585) still unconditionally setsextended_store_plan.values_query.is_exact_query = false, regardless of the aggregation's actualWindowType. This is exactly problem #2 named in #580, which PR #582 closed — but #582's changes (keys_query expansion, same-timestamp bucket collision) never touched this line. The bug is still live on main.Effect: a PromQL range query over a Sliding-window aggregation gets routed to the overlap-scan fetch (
query_precomputed_output) instead of the exact-window fetch (query_precomputed_output_exact), and is then merged downstream as if it were Tumbling (window_type = if is_exact_query {Sliding} else {Tumbling},simple_engine/mod.rs:545) — wrong values, silently, no error.Instant queries don't have this bug:
create_store_query_plancorrectly derivesis_exact_query = window_type == WindowType::Sliding(simple_engine/mod.rs:414). Only the range path hardcodes it away.Proposal to consider as part of this unification: instead of re-deriving
is_exact_querycorrectly in the range path (mirroring case 1), consider removingis_exact_queryas a threaded bool field entirely — infer which store lookup to call (query_precomputed_outputvs_exact) directly fromWindowTypeat the fetch call site, in both the instant and range paths. Would remove one more place the two paths can independently drift out of sync, on top of the fetch/merge unification this issue already covers.Staged unification plan:
- Align call order between the two pipelines (fetch values → fetch+merge keys → merge values → ...) — zero behavior change. Also rename range's
window_mode(sliding/hopping step-iteration label) so it stops colliding in name with instant'swindow_type(Sliding/Tumbling store-fetch mode). - Extract range's inline logic into named helpers mirroring instant's —
groupsresolution → shared withcollect_results_separate_keys/collect_results_same_aggregation; bucket-map-build + window-walk → its own helper. Zero behavior change; makes the two pipelines diff-able side by side. - Unify the merge engine —
merge_accumulators(KLL/CMS batch-merge fast path) vsNaiveMerger(always sequential,slide()unused). The one real behavior fork; needs equivalence tests before either call site changes, own commit. - Collapse instant into range —
execute_query_pipelinebecomesexecute_range_query_pipelinewith a single step. The actual unification; 1-3 just make it reviewable.
Must resolve before/during step 4, not after: Sliding forced-false bug in range fetch, top-k has no range semantics yet, partial-data/error tolerance differs between the two, SQL/Elastic call
execute_query_pipelinedirectly so their call sites are in scope too.- Align call order between the two pipelines (fetch values → fetch+merge keys → merge values → ...) — zero behavior change. Also rename range's
Scoping recap after #583/#596/#597 landed and #607/#608/#609 were filed (fetch-mode correctness bug for Sliding-window range queries + its lock/perf prerequisites — orthogonal to this issue's remaining scope, no ordering dependency either way, just possible file-level rebase overlap in
execute_range_query_pipeline's per-step loop).Remaining scope for #581, updated for current state:
# Item Status Risk A Rename range's window_modedebug-log local (mod.rs:1592) — collides in name with instant'swindow_type(WindowType::Sliding/Tumbling, picks the store call);window_modeis an unrelated "sliding vs hopping" step-iteration label. Zero behavior change.Not started trivial B Factor range's inline KeysSourceenum (mod.rs:1638, Fixed/PerStep) into shared helpers with instant'scollect_results_separate_keys/collect_results_same_aggregation. #583 converged the shapes but didn't extract shared code.Not started medium C Merge-engine unification: merge_accumulators(KLL/CMS batch-merge fast path, warn-and-continue on error) vsNaiveMerger(sequential-only, abort on error).Tracked separately as #596, open, not started high — real behavior fork, needs equivalence tests D Product decisions: top-k semantics for range (per-step vs whole-range?), whether a collapsed "step=1" should error-strict like instant or tolerate-gaps like range today, SQL/Elastic entry points are instant-only. Not decided blocks stage 4, not code work yet E Collapse: execute_query_pipelinebecomesexecute_range_query_pipelinecalled with one step.Not started, depends on B/C/D highest Note: the original 4-stage plan's "align call order" (first half of stage 1) is now largely moot — pre-#583, range eagerly merged keys once (mirroring instant) before the per-step loop; #583 removed that (it was itself the bug #583 fixed) so range now defers keys merging into the per-step loop, same as values. Instant still merges once, correctly, since it has exactly one output step. That's the single-step-vs-N-step distinction stage E is about, not drift — only the naming-collision rename (row A) is still worth doing as flat renaming.
- added a commit that references this issue
on Aug 25, 2026 Groundwork session decisions (B, D):
- C = Range query merges lack the CMS/KLL batch-merge fast path instant queries get #596, tracked/fixed separately. Note: Range query merges lack the CMS/KLL batch-merge fast path instant queries get #596 isn't pure perf — CountMinSketch's fast path is exact-equivalent to sequential merge, but DatasketchesKLL's native batch merge can produce numerically different (both validly-bounded) quantiles than sequential pairwise merge. Worth reflecting in Range query merges lack the CMS/KLL batch-merge fast path instant queries get #596 directly.
- B: one shared, parameterized key-resolution function called by both pipelines (instant once, range once per step) — not two mirrored-but-separate functions. This also closes the audit doc's "range can't check a self-keyed value accumulator" gap for free, since the resolver takes an already-merged value accumulator as input.
- D1 (top-k): per-step, matching real PromQL
topk()semantics — no new cross-step state needed. - D2 (SQL/Elastic): no changes.
execute_query_pipelinekeeps its exact signature/return type as a thin single-step wrapper; only PromQL's range handler and shared internals change. - D (partial-data semantics) and D (binary-expr call sites): turned out to be non-issues — already behaviorally consistent between the two pipelines once considered carefully (partial-data: both already tolerate physical-bucket gaps and only hard-fail on a truly-empty whole-query fetch), and non-issues by construction once D2 holds (binary-expr arms just call the unchanged wrapper).
Next: E's mechanical shape.
E's mechanical shape, decided:
- Unified engine parameterized by an explicit
output_timestamps: &[u64], not start/end/step_ms. Range computes the list upstream (promql.rs); instant passes a 1-element list. No dummy/unused step_ms for the instant case. execute_query_pipelinebecomes a thin wrapper: calls the unified engine with one timestamp, unwraps the single-sampleRangeVectorElementintoInstantVectorElement. Its public signature/return type is unchanged (see D2 above — SQL/Elastic need no changes).- Fetch-side check: a single-step call's window computation already reduces exactly to instant's current Tumbling window math. For Sliding it only reduces correctly once Range queries over Sliding-window aggregations use overlap-scan fetch, not exact-window fetch #608 lands — otherwise collapse would regress Sliding-window instant queries onto range's (buggy) scan fetch.
- Per-step top-k (D1) requires ranking across all groups at a given timestamp before truncating — today's loop is group-major (all steps for group A, then all steps for group B, ...) and can't do that. Restructuring to step-major (all groups for t1, rank/truncate, then t2, ...) is required for correctness, not optional. Decided: always step-major, one loop shape for top-k and non-top-k alike — two loop implementations would just reintroduce the kind of duplication Unify PromQL instant and range query fetch/merge paths #581 exists to remove. Per-group setup (bucket_map, keys_source) still happens once per group, precomputed before the step-outer loop.
Sequencing: E depends on B, #596, and #608 all landing first (all three touch the same functions E would restructure). Decided to wait for all three rather than start E's loop restructure in isolation, despite it being mechanically separable — avoids rebasing E through three more landings in the same code.
This closes out the groundwork/scoping session for #581's B-E rows.
- Unified engine parameterized by an explicit
- added 5 commits that reference this issue
on Aug 25, 2026 Remaining incremental-merging work is in #641
PromQL instant queries and range queries use two independently implemented fetch/merge designs for the same underlying job, not one design expressed twice.
values_query(exact-window if Sliding, range if Tumbling), merged once viamerge_precomputed_outputs(fold overmerge_with, with ado_mergeshort-circuit when only one bucket is in range).WindowMerger/NaiveMergerbuilt and fully re-merged from scratch per output step, per key. The trait's incrementalslide()(add/remove) is never used.Binary-expr instant and range queries each just run the corresponding plain-query pipeline once per arm and combine client-side, so this fork is duplicated across all four PromQL query paths, not just the two plain ones.