Skip to content

feat(query-engine): cut over PromQL binary-expr instant queries to native execution - #573

Closed
milindsrivastava1997 wants to merge 10 commits into
567-2-native-instant-binary-evaluatorfrom
567-3-cutover
Closed

feat(query-engine): cut over PromQL binary-expr instant queries to native execution#573
milindsrivastava1997 wants to merge 10 commits into
567-2-native-instant-binary-evaluatorfrom
567-3-cutover

Conversation

@milindsrivastava1997

@milindsrivastava1997 milindsrivastava1997 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

This is the final stage of #567 — the whole query-serving surface of asap-query-engine's SimpleEngine now runs on a single (native) engine. Stacked on #572 (Stage 2), which is stacked on #570 (Stage 1).

Test plan

  • native_binary_instant_tests.rs rewritten (nothing left to compare against DataFusion for this path): all-ops, power operator, scalar both-orderings, nested binary, empty-arm fallback, unsupported-arm, dual-population, plus two new Stage-3-specific tests — binary_expr_sliding_window_end_to_end_merges_correctly (ties fix(query-engine): merge all sliding-window buckets per key instead of taking first #570's fix to the real production entrypoint) and binary_expr_works_on_current_thread_runtime (only passes because block_in_place is actually gone)
  • dispatch_arithmetic_tests.rs / plan_execution_arithmetic_tests.rs re-run unmodified as black-box regression guards through the new native path
  • Full cargo test --lib (541 tests) green
  • cargo clippy --lib --tests -- -D warnings clean

Closes #567 once this stack merges to main.

@milindsrivastava1997
milindsrivastava1997 force-pushed the 567-2-native-instant-binary-evaluator branch from 22d0f97 to ce9dc1f Compare August 23, 2026 02:02
milindsrivastava1997 and others added 10 commits August 22, 2026 22:46
@milindsrivastava1997 @claude
fix(query-engine): merge all sliding-window buckets per key instead o…
b5d7e3a 
…f taking first

execute_and_merge_store_queries kept only the first precomputed bucket
per key for Sliding-window queries, discarding the rest when the store
returned more than expected. DataFusion's SummaryMergeMultipleExec
already merges all of them correctly, so binary-expr queries (still on
DataFusion) don't hit this. #567 will move binary-expr onto this native
path, so this native/DataFusion behavior gap needed closing first.

Part of #567 Stage 1.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 @claude
fix(query-engine): dedupe sliding-window merge path, fix double-clone…
1df67d4 
… and stale latency label

- Sliding branch now delegates to merge_precomputed_outputs (do_merge=true)
  instead of hand-rolling its own extract/merge/insert loop, removing the
  duplication with the Tumbling branch's merge path.
- merge_accumulators now takes ownership of the accumulator Vec so its
  single-element shortcut can move the value out instead of re-cloning it
  on top of the clone already done to build the Vec.
- The [LATENCY] log's merge/no-merge label was hardcoded off window_type
  and said "no merge" even when merge_accumulators was in fact called;
  it now reflects whether merging actually occurs.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 @claude
fix(query-engine): warn instead of silently dropping keys with no pre…
8e768fa 
…compute buckets

merge_precomputed_outputs silently skipped any key whose timestamped_buckets
list was empty, with no log signal — unlike the "found N, expected 1"
mismatch case a few lines up in the Sliding caller, which does warn. Since
this function is shared by Sliding, Tumbling, and the keys-merge path, the
warn now covers all three instead of being Sliding-only.

Also files #575 to compute EXPECTED_BUCKETS_PER_KEY instead of hardcoding it
to 1, since #554 will make >1 legitimate whenever a sliding-window query's
range exceeds the window size.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 @claude
feat(query-engine): add native instant binary-expr evaluator, staged …
8311f60 
…ahead of cutover

Builds a native (non-DataFusion) implementation of PromQL binary-
arithmetic instant queries: a recursive arm evaluator over
Vec<InstantVectorElement>, plus vector-vector and scalar combiners
lifted from the existing range-binary path. Not yet wired into
production dispatch (handle_query_promql still calls the DataFusion
path) — exposed via handle_query_promql_native for equivalence
testing against the DataFusion path ahead of the Stage 3 cutover.

One accepted, deliberately loud behavior change: an arm with zero
current precomputed data now falls back to Prometheus (matching
"not acceleratable" semantics) instead of DataFusion's silent
empty-result behavior, with a warn! so it's observable.

Part of #567 Stage 2.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 @claude
fix(query-engine): reject cross-label-set joins in native binary-expr…
90128c8 
… combiner

combine_vector_vector_native joined two arms purely by positional
KeyByLabelValues equality (a bare Vec<String> of values, no label
names attached). DataFusion's build_binary_vector_plan joins on named
columns instead, so it fails to resolve (-> None) whenever the two
arms don't share the same label set. Native had no equivalent check,
so it could silently fabricate a joined result whenever two
differently-grouped arms' values happened to coincide (e.g.
`sum(a) by (host) + sum(b) by (region)` with a host value equal to a
region value).

combine_vector_vector_native now takes both arms' label-name lists
and returns None on a mismatch, matching DataFusion's failed-join
behavior. Adds four regression tests proving the divergence (three
were red before this fix) plus a same-label-set control case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 @claude
fix(query-engine): enable topk limiting/formatting for binary-expr le…
d86abd6 
…af arms

evaluate_arm_native's leaf branch hardcoded execute_query_pipeline(&ctx,
false, false), so a topk arm inside a binary expression (e.g. `topk(10,
metric) + 0`) never got truncated to k or metric-name-prefixed — both flags
are self-gated on statistic == Topk / a "k" kwarg being present (see
execute_query_pipeline's doc comment), so passing (true, true)
unconditionally is a no-op for non-topk arms, matching what the main
non-binary instant-query path already does.

Adds a regression test proving the divergence: topk(10, ...) + 0 returned
all 15 unformatted rows before this fix, now correctly truncates to 10 with
the metric-name prefix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 @claude
feat(query-engine): cut over PromQL binary-expr instant queries to na…
eb2fd25 
…tive execution

Rewires handle_binary_expr_promql to call the native evaluator/
combiners (built in the prior stage) instead of building a DataFusion
plan. Deletes the old DataFusion-based handle_binary_expr_promql,
build_arm_logical_plan, and the tokio::task::block_in_place(...
block_on(...)) wrapper it needed (native execution is synchronous).
Renames evaluate_arm_native/combine_vector_vector_native/
combine_scalar_native -> evaluate_binary_arm/combine_vector_vector/
combine_scalar now that native is the only implementation, and drops
the now-redundant handle_binary_expr_promql_native/
handle_query_promql_native test-only wrappers.

This was the only production code path still reachable through
DataFusion (asap-query-engine's SimpleEngine now serves every query
shape through the same native fetch/merge pipeline). DataFusion's
CustomQueryPlanner/PrecomputedSummaryReadExec/SummaryMergeMultipleExec/
build_binary_vector_plan/build_scalar_plan and the datafusion
dependency itself are left in place, per #567's scope — still
exercised by their own dedicated tests and the unwired execute_plan
prototype, just no longer reachable from production.

Closes #567 Stage 3 (final stage).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997
fix(query-engine): address code-review cleanup findings on PR #567 St…
9d43776 
…age 3 cutover

- move plan_execution_arithmetic_tests.rs out of tests/datafusion/ since it
  now exercises the native binary-expr path, not DataFusion
- fix warn! in evaluate_binary_arm that mislabeled any execute_query_pipeline
  error as "produced no results"
- mark orphaned execute_logical_plan #[allow(dead_code)] and update its doc
  comment, matching its sibling execute_plan
- avoid an unnecessary Vec<String> clone in combine_vector_vector
- mark design-252 doc as superseded by the native cutover in #567
@milindsrivastava1997 @claude
fixup(query-engine): update topk-in-binary-expr test for post-cutover…
bc580c2 
… API

handle_query_promql_native was removed by the #567 Stage 3 cutover (native
is now the only path, folded into handle_query_promql) — this commit was
squashed into the branch during rebase onto the cutover.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997 @claude
fix(query-engine): reject cross-label-set joins in range binary-expr;…
b9287d8 
… fix stale dead-code comment

handle_binary_expr_range_promql's vector-vector join matched purely on
positional KeyByLabelValues equality (rhs labels discarded), unlike the
instant-query combine_vector_vector fixed earlier in this stack (#572) to
reject a join between arms grouped by different label sets. Two arms grouped
by disjoint labels (e.g. (host) vs (region)) whose values happened to
coincide could silently join into a wrong-but-plausible result across the
whole range. Now rejects the join (returns None) when lhs_labels !=
rhs_labels, mirroring the instant-query guard.

Extends create_range_engine_two_metrics (range_query_arithmetic_tests.rs) to
take per-metric grouping labels, and adds a regression test proving the
divergence.

Also corrects execute_logical_plan's doc comment: it claimed to be "part of
the still-exercised DataFusion path (see its dedicated tests)", but it has
zero callers anywhere in the repo, including tests -- unlike its sibling
execute_plan, which genuinely is still called by DataFusion-path tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@milindsrivastava1997
milindsrivastava1997 force-pushed the 567-2-native-instant-binary-evaluator branch from defd004 to d86abd6 Compare August 23, 2026 02:46
@milindsrivastava1997
milindsrivastava1997 force-pushed the 567-2-native-instant-binary-evaluator branch from d86abd6 to f814b6c Compare August 23, 2026 02:53
@milindsrivastava1997
milindsrivastava1997 deleted the branch 567-2-native-instant-binary-evaluator August 23, 2026 02:54
@milindsrivastava1997 milindsrivastava1997 mentioned this pull request Aug 23, 2026
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@milindsrivastava1997