Repository navigation
Description
Context
asap-query-engine's SimpleEngine currently serves PromQL binary-arithmetic
instant queries (vector-vector and vector-scalar) through a dedicated
DataFusion path (handle_binary_expr_promql → build_arm_logical_plan →
build_binary_vector_plan/build_scalar_plan → execute_logical_plan), while
every other live query shape (plain vector-selector instant queries, both
PromQL range-query shapes, all of SQL, all of Elastic DSL) is served by the
native/direct path (execute_query_pipeline → execute_and_merge_store_queries
→ Store::query_precomputed_output[_exact] + hand-written merge).
This is the only place DataFusion is reachable in production at all. Having
one query shape run through a structurally different engine means the two
implementations of "fetch precomputed outputs and merge them" have to be kept
behaviorally equivalent by hand — and they already aren't (see design
decisions below).
Full audit this issue was scoped from: .design_docs/query-engine-fetch-merge-audit.md.
Design decisions (resolved via grilling session, 2026-08-21)
-
Sliding-bucket merge bug is real and must be fixed first, its own stage.
execute_and_merge_store_queries(simple_engine/mod.rs:551-567) takes the
first bucket per key for Sliding-window queries and discards the rest
(warn-only).SummaryMergeMultipleExec(DataFusion) merges all of them
correctly. Binary-expr queries are currently the one shape immune to this,
because they're the one shape still on DataFusion — cutting them over
as-is would silently inherit the bug. DataFusion's merge-all behavior is
the correct one; native gets fixed to match, not the other way round.
Expected-count-per-key is hardcoded to1for now (ponytail:comment
pointing at Sliding window execution: support multi-window merge/subtract in query engine #554, which will make >1 legitimate — don't block on it).
Done: fix(query-engine): merge all sliding-window buckets per key instead of taking first #570. -
Label-order join "bug" investigated, no live bug found — dropped.
Originally scoped as its own stage: the vector-vector join
(handle_binary_expr_range_promql) keys aHashMapby raw
KeyByLabelValues(positionalVec<String>), and the concern was that two
metrics sharing a label set could declare it in different order, silently
breaking the join. Turns out this can't happen:KeyByLabelNames::new()
(promql_utilities/data_model/key_by_label_names.rs:10-18)
unconditionally sorts label names, and every source of label-name ordering
reachable from a PromQL binary expression —PromQLSchema.get_labels(),
and theby(...)/without(...)clause derivation
(get_spatial_aggregation_output_labels) — routes through it. Confirmed
with an isolated scratch test (two metrics, deliberately reversed
declaration order, both come back canonically sorted).
One real bypass exists —asap-planner-rs/src/planner/elastic_dsl.rs:126-129
constructsKeyByLabelNamesvia struct literal, unsorted, from the
Elastic DSL query's owngroup_byclause order — but it's unreachable
from this join: Elastic DSL has no binary-expr concept, and aSimpleEngine
instance is permanently locked to oneQueryLanguageat construction
(simple_engine/mod.rs:1040-1044), so an engine holding Elastic-planned
(possibly unsorted)AggregationConfigs never routes through
handle_binary_expr_promql/handle_binary_expr_range_promqlregardless of
input. See Elastic DSL's unsorted KeyByLabelNames construction would break if binary arm support is ever added #571 for the forward-looking risk if Elastic ever gains binary
arm support. -
Nested binary arms (
(a+b)*c) must be preserved in the native instant
evaluator. DataFusion'sbuild_arm_logical_planrecurses on nested
Binaryarms today (promql.rs:379-387); the range-binary path being
borrowed from does not (tracked separately as promql.rs: range queries don't accelerate nested binary arithmetic #516, out of scope here).
The native instant evaluator needs its own recursion so this doesn't
regress. -
DataFusion removal from the crate is out of scope for asap-query-engine: convert binary PromQL instant queries from DataFusion to native execution #567.
CustomQueryPlanner/PrecomputedSummaryReadExec/SummaryMergeMultipleExec/
build_binary_vector_plan/build_scalar_plan/thedatafusiondependency,
and the unwiredexecute_planprototype (mod.rs:717) that still uses them
in tests, all stay as-is. asap-query-engine: convert binary PromQL instant queries from DataFusion to native execution #567 only stops production from calling
DataFusion. Deletion is a deliberate follow-up once the native path has
proven itself, not bundled into this cutover. -
TDD, red tests first, every stage. All tests land as a permanent,
committed suite (including the old-DataFusion-vs-new-native equivalence
checks) — not a throwaway local proof. This supersedes the usual
"local-only equivalence check, don't commit it" default for critical
refactors, specifically because this work is fixing real bugs and adding
new permanent behavior, not just re-deriving something that already existed. -
Stage boundaries are commit/pause points. Each stage lands
independently (build/test/clippy green) and stops for manual commit —
no auto-continuing through the plan.
Staged plan
Stage 1 — fix the sliding-bucket merge bug (simple_engine/mod.rs:551-567)
Merge all buckets per key instead of taking the first; warn (not drop) when
count ≠ expected(1). Done: #570.
Stage 2 — build the native instant binary evaluator
- Leaf resolution: swap
ctx.to_logical_plan()for
execute_query_pipeline(&ctx, false, false)(promql.rs:389-391) →
Vec<InstantVectorElement>instead ofLogicalPlan. - Native recursive arm evaluator mirroring
build_arm_logical_plan
(promql.rs:368-394): leaf → pipeline result; nestedBinary→ recurse
both sides then combine;Paren→ recurse;NumberLiteral→None. - Native vector-vector combiner, lifted from the range binary path
(promql.rs:585-620) — positionalKeyByLabelValuesjoin is fine as-is
(see label-order design decision above). - Native scalar combiner, lifted from the range binary scalar branch
(promql.rs:569-582);apply_range_binary_op(promql.rs:457-472) is
reusable as-is (maybe rename, "range" no longer accurate).
Stage 3 — cutover
Rewire handle_binary_expr_promql (promql.rs:400-454): keep
detect_scalar_arm dispatch, swap the DataFusion-plan-building calls for the
new native evaluator/combiners, delete the
tokio::task::block_in_place(...block_on(execute_logical_plan(...)))
wrapper (native execution is synchronous, no async/blocking dance needed).
Tests (TDD, red before implementation, all permanent)
Stage 1 — src/tests/native_pipeline_merge_tests.rs (done, #570):
sliding_single_bucket_returns_its_value— 1 bucket, baseline unchanged.sliding_two_buckets_for_same_key_are_merged_not_dropped— 2 buckets → merged, not first-only. (red test for today's bug)sliding_bucket_count_mismatch_still_returns_merged_result— count ≠ 1 still merges + warns, doesn't drop.tumbling_multi_bucket_merge_unaffected_by_sliding_fix— Tumbling branch (already correct) untouched.
Stage 2 — src/tests/native_binary_instant_tests.rs:
5. native_vector_vector_all_ops_match_datafusion — table-driven old-vs-new equivalence across + - * / % ^.
6. native_vector_scalar_both_orderings_match_datafusion — 2*foo and foo*2 both match DataFusion.
7. native_nested_binary_matches_datafusion — (foo+bar)*baz matches DataFusion. (nested-arm parity)
8. native_binary_expr_no_data_falls_back_to_none — one arm's store query errors → None (Prometheus fallback), no panic.
9. native_binary_expr_unsupported_arm_returns_none — non-acceleratable arm → None, unchanged from today.
10. native_vector_vector_dual_population_matches_datafusion — one arm is dual-population (separate keys_query), old vs new match.
Stage 3 — extends native_binary_instant_tests.rs:
11. sliding_window_binary_expr_end_to_end_merges_correctly — through real handle_query_promql: Sliding-window binary-expr query with >1 bucket/key returns the fully-merged value.
12. native_binary_expr_works_on_current_thread_runtime — run under #[tokio::test] (single-threaded, not multi_thread) — only passes once block_in_place is actually gone.
13. Existing dispatch_arithmetic_tests.rs suite re-run unmodified as a black-box regression guard.
Activity
milindsrivastava1997 commentedon Aug 21, 2026
Considered whether the range-query pipeline could just be reused directly for
instant queries (both usual and binary arms) instead of writing the native
evaluator/combiners in the steps above. It's blocked in a few concrete ways:
start == endis explicitly rejected.validate_range_query_params(
mod.rs:1292) hasif start >= end { return Err("start must be before end") }.An instant query is exactly
start == end, so you can't call the rangepipeline with the true instant point at all — you'd have to fake a tiny
non-zero width, which changes what's actually being evaluated (last point of
a synthetic small range, not "at this instant"), and
stepalso has to be apositive multiple of
tumbling_window_ms(:1298), so you can't even pick anarbitrary trivial step.
execute_range_query_pipelinenever looks atkeys_queryat all. Grepfor
keys_queryacrossmod.rsturns up exactly one function body thattouches it —
execute_and_merge_store_queries(:591), the native instantpath. The range pipeline only ever fetches
context.base.store_plan.values_query(
:1431); there's no code path in it that fetches or merges a separate keysaggregation. So any dual-population metric (
MultipleSum/MultipleMinMax/HydraKLLpaired with aDeltaSetAggregatorkeys aggregation) would silentlydrop the keys side if routed through range. This isn't a missing detail, it's
a missing feature.
Sliding windows are structurally incompatible with the range pipeline,
same issue as the sliding-window doc flags elsewhere: it forces
is_exact_query = falseand grid-steps bytumbling_window_msunconditionally (
promql.rs:1141-1144), which is exactly backwards forSliding aggregations (single exact-window fetch, no grid). Routing
instant-Sliding queries through range would hit the same mismatch already
flagged in the sliding-window doc.
feat(query-engine): cut over PromQL binary-expr instant queries to na…
fix(query-engine): address code-review cleanup findings on PR #567 St…
feat(query-engine): add native instant binary-expr evaluator, staged …
feat(query-engine): cut over PromQL binary-expr instant queries to na…
fix(query-engine): address code-review cleanup findings on PR #567 St…
fixup(query-engine): update topk-in-binary-expr test for post-cutover…
fix(query-engine): merge all sliding-window buckets per key instead o…
feat(query-engine): add native instant binary-expr evaluator, staged …
feat(query-engine): cut over PromQL binary-expr instant queries to na…
fix(query-engine): address code-review cleanup findings on PR #567 St…
fixup(query-engine): update topk-in-binary-expr test for post-cutover…
fix(query-engine): merge all sliding-window buckets per key instead o…
feat(query-engine): add native instant binary-expr evaluator, staged …
feat(query-engine): add native instant binary-expr evaluator, staged …
feat(query-engine): cut over PromQL binary-expr instant queries to na…
fix(query-engine): address code-review cleanup findings on PR #567 St…
fixup(query-engine): update topk-in-binary-expr test for post-cutover…
feat(query-engine): cut over PromQL binary-expr instant queries to na…
fix(query-engine): range queries expand keys_query and merge same-tim…
refactor(query-engine): remove unused DataFusion query engine
refactor(query-engine): remove unused DataFusion query engine (#615)