Skip to content

Commit 19d4c77

Browse files
fix(query-engine): address code-review cleanup findings on PR #567 Stage 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
1 parent 32f2781 commit 19d4c77

6 files changed

Lines changed: 18 additions & 6 deletions

File tree

asap-query-engine/src/engines/simple_engine/mod.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -823,8 +823,11 @@ impl SimpleEngine {
823823

824824
/// Executes a pre-built DataFusion logical plan and returns results.
825825
///
826-
/// This is the shared execution kernel used by both `execute_plan` (for single-metric
827-
/// queries) and the binary arithmetic dispatch path.
826+
/// This was the shared execution kernel for `execute_plan` and the DataFusion-based
827+
/// binary arithmetic dispatch path; the latter was cut over to a native implementation
828+
/// in #567, leaving this unused in production. Kept alongside `execute_plan` as part
829+
/// of the still-exercised DataFusion path (see its dedicated tests).
830+
#[allow(dead_code)]
828831
pub async fn execute_logical_plan(
829832
&self,
830833
logical_plan: datafusion::logical_expr::LogicalPlan,

asap-query-engine/src/engines/simple_engine/promql.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -66,9 +66,9 @@ fn combine_vector_vector(
6666
lhs_results
6767
.into_iter()
6868
.filter_map(|lhs_elem| {
69-
rhs_map.get(&lhs_elem.labels).map(|&rhs_val| {
69+
rhs_map.get(&lhs_elem.labels).map(move |&rhs_val| {
7070
let value = SimpleEngine::apply_range_binary_op(op, lhs_elem.value, rhs_val);
71-
InstantVectorElement::new(lhs_elem.labels.clone(), value)
71+
InstantVectorElement::new(lhs_elem.labels, value)
7272
})
7373
})
7474
.collect(),
@@ -469,7 +469,7 @@ impl SimpleEngine {
469469
.execute_query_pipeline(&ctx, true, true)
470470
.map_err(|e| {
471471
warn!(
472-
"Binary-expr arm for metric '{}' produced no results ({}) — \
472+
"Binary-expr arm for metric '{}' failed ({}) — \
473473
falls back to Prometheus for the whole expression rather than \
474474
returning an empty result for just this arm",
475475
ctx.metric, e

asap-query-engine/src/tests/datafusion/mod.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ pub mod accumulator_serde_tests;
77
pub mod dispatch_arithmetic_tests;
88
pub mod plan_builder_binary_tests;
99
pub mod plan_builder_regression_tests;
10-
pub mod plan_execution_arithmetic_tests;
1110
pub mod plan_execution_dual_input_tests;
1211
pub mod plan_execution_temporal_tests;
1312
pub mod plan_execution_tests;

asap-query-engine/src/tests/mod.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ pub mod clickhouse_forwarding_tests;
33
pub mod datafusion;
44
pub mod elastic_dsl_query_tests;
55
pub mod elastic_forwarding_tests;
6+
pub mod native_binary_arithmetic_plan_tests;
67
pub mod native_binary_instant_tests;
78
pub mod native_pipeline_merge_tests;
89
pub mod prometheus_forwarding_tests;

asap-query-engine/src/tests/datafusion/plan_execution_arithmetic_tests.rs renamed to asap-query-engine/src/tests/native_binary_arithmetic_plan_tests.rs

File renamed without changes.

docs/design-252-arithmetic-operators.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,14 @@
11
# Design: PromQL Arithmetic Operator Acceleration (Issue #252)
22

3+
> **Superseded (#567):** the DataFusion execution path described below
4+
> (`execute_plan`/`execute_logical_plan`, the `Join + Projection` plan) was
5+
> replaced by a purely native execution path in `handle_binary_expr_promql` /
6+
> `evaluate_binary_arm` / `combine_vector_vector` / `combine_scalar`
7+
> (`asap-query-engine/src/engines/simple_engine/promql.rs`). The DataFusion
8+
> code is kept only for its own dedicated tests and is no longer reachable
9+
> from production. This doc is retained as the historical record of the
10+
> original design decision.
11+
312
## Problem
413

514
ASAPQuery accelerates PromQL queries by pre-computing sketches over streaming data and serving answers from those sketches at query time, bypassing the underlying TSDB for supported query patterns.

0 commit comments

Comments
 (0)