feat(query-engine): cut over PromQL binary-expr instant queries to native execution - #577
Merged
Conversation
…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>
…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
… 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>
… 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>
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
1 participant
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.
Summary
handle_binary_expr_promqlto call the native evaluator/combiners (built in feat(query-engine): add native instant binary-expr evaluator, staged ahead of cutover #572) instead of building a DataFusion plan.handle_binary_expr_promql,build_arm_logical_plan, and thetokio::task::block_in_place(...block_on(...))wrapper it needed — native execution is synchronous.evaluate_arm_native/combine_vector_vector_native/combine_scalar_native→evaluate_binary_arm/combine_vector_vector/combine_scalarnow that native is the only implementation, and drops the now-redundanthandle_binary_expr_promql_native/handle_query_promql_nativetest-only wrappers from feat(query-engine): add native instant binary-expr evaluator, staged ahead of cutover #572.CustomQueryPlanner/PrecomputedSummaryReadExec/SummaryMergeMultipleExec/build_binary_vector_plan/build_scalar_plan, thedatafusiondependency, and the unwiredexecute_planprototype are all left in place per asap-query-engine: convert binary PromQL instant queries from DataFusion to native execution #567's scope — still exercised by their own dedicated tests, just no longer reachable from production.handle_binary_expr_range_promql's vector-vector join now rejects mismatched label sets (mirroring the instant-query guard from feat(query-engine): add native instant binary-expr evaluator, staged ahead of cutover #572), andexecute_logical_plan's doc comment no longer falsely claims test coverage it doesn't have.This is the final stage of #567 — the whole query-serving surface of
asap-query-engine'sSimpleEnginenow runs on a single (native) engine. #570 (Stage 1) and #572 (Stage 2) have both merged tomain; this PR is retargeted directly atmain(replaces #573, which got closed when the branches were unstacked — same content, rebased).Test plan
native_binary_instant_tests.rsrewritten (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 Stage-3-specific tests tying fix(query-engine): merge all sliding-window buckets per key instead of taking first #570's fix to the real production entrypointtest_range_vector_vector_mismatched_label_sets_return_none)topk_wrapped_in_binary_expr_still_truncates_and_formats)dispatch_arithmetic_tests.rs/plan_execution_arithmetic_tests.rsre-run unmodified as black-box regression guards through the new native pathcargo test --lib(547 tests) greencargo clippy --lib --tests -- -D warningscleanCloses #567.