fix(query-engine): reject cross-label-set joins in range binary-expr; 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>

Author: milindsrivastava1997

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

@@ -823,10 +823,12 @@ impl SimpleEngine {
 
     /// Executes a pre-built DataFusion logical plan and returns results.
     ///
-    /// This was the shared execution kernel for `execute_plan` and the DataFusion-based
-    /// binary arithmetic dispatch path; the latter was cut over to a native implementation
-    /// in #567, leaving this unused in production. Kept alongside `execute_plan` as part
-    /// of the still-exercised DataFusion path (see its dedicated tests).
+    /// This was the entry point for the DataFusion-based binary arithmetic
+    /// dispatch path, cut over to a native implementation in #567. Unlike its
+    /// sibling `execute_plan` (still called by DataFusion-path tests), this
+    /// function has zero callers anywhere in the repo, including tests — it
+    /// is genuinely dead code, kept only in case the native cutover needs to
+    /// be reverted.
     #[allow(dead_code)]
     pub async fn execute_logical_plan(
         &self,

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

@@ -651,9 +651,17 @@ impl SimpleEngine {
             return Some((KeyByLabelNames::new(labels), QueryResult::matrix(combined)));
         }
 
-        // Vector-vector: evaluate both arms, join by label key, apply op per matching timestamp
+        // Vector-vector: evaluate both arms, join by label key, apply op per matching timestamp.
+        // Reject mismatched label sets up front — same guard as the instant-query
+        // combine_vector_vector, and for the same reason: positional
+        // KeyByLabelValues equality below is only safe once the label *names*
+        // match (they're canonically sorted by KeyByLabelNames::new(), so two
+        // arms with the same label set always order their values the same way).
         let (lhs_ctx, lhs_labels) = self.build_arm_range_context(lhs, start, end, step)?;
-        let (rhs_ctx, _) = self.build_arm_range_context(rhs, start, end, step)?;
+        let (rhs_ctx, rhs_labels) = self.build_arm_range_context(rhs, start, end, step)?;
+        if lhs_labels != rhs_labels {
+            return None;
+        }
         let lhs_results = self.execute_range_query_pipeline(&lhs_ctx).ok()?;
         let rhs_results = self.execute_range_query_pipeline(&rhs_ctx).ok()?;
 

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

@@ -36,18 +36,22 @@ mod tests {
     /// `engine_factories::create_engine_two_metrics` (single timestamp, instant
     /// queries only), this inserts one bucket per `(timestamp, value)` pair so
     /// range queries have more than one output sample to join across.
+    #[allow(clippy::too_many_arguments)]
     fn create_range_engine_two_metrics(
         metric_a: &str,
+        labels_a: Vec<&str>,
         data_a: TimeSeriesData,
         query_a: &str,
         metric_b: &str,
+        labels_b: Vec<&str>,
         data_b: TimeSeriesData,
         query_b: &str,
     ) -> SimpleEngine {
-        let labels = vec!["host".to_string()];
+        let labels_a: Vec<String> = labels_a.iter().map(|s| s.to_string()).collect();
+        let labels_b: Vec<String> = labels_b.iter().map(|s| s.to_string()).collect();
 
         let mut aggregation_configs = HashMap::new();
-        for (id, metric) in [(1u64, metric_a), (2u64, metric_b)] {
+        for (id, metric, labels) in [(1u64, metric_a, &labels_a), (2u64, metric_b, &labels_b)] {
             aggregation_configs.insert(
                 id,
                 AggregationConfig {
@@ -91,8 +95,8 @@ mod tests {
         }
 
         let promql_schema = PromQLSchema::new()
-            .add_metric(metric_a.to_string(), KeyByLabelNames::new(labels.clone()))
-            .add_metric(metric_b.to_string(), KeyByLabelNames::new(labels));
+            .add_metric(metric_a.to_string(), KeyByLabelNames::new(labels_a))
+            .add_metric(metric_b.to_string(), KeyByLabelNames::new(labels_b));
 
         let inference_config = InferenceConfig {
             schema: SchemaConfig::PromQL(promql_schema),
@@ -143,9 +147,11 @@ mod tests {
         let data_requests = host_a_series([(1000, 200.0), (2000, 300.0)]);
         let engine = create_range_engine_two_metrics(
             "errors_total",
+            vec!["host"],
             data_errors,
             "sum(errors_total) by (host)",
             "requests_total",
+            vec!["host"],
             data_requests,
             "sum(requests_total) by (host)",
         );
@@ -174,9 +180,11 @@ mod tests {
         let data_b = host_a_series([(1000, 20.0), (2000, 25.0)]);
         let engine = create_range_engine_two_metrics(
             "metric_a",
+            vec!["host"],
             data_a,
             "sum(metric_a) by (host)",
             "metric_b",
+            vec!["host"],
             data_b,
             "sum(metric_b) by (host)",
         );
@@ -199,10 +207,12 @@ mod tests {
         let data_a = host_a_series([(1000, 5.0), (2000, 6.0)]);
         let engine = create_range_engine_two_metrics(
             "metric_a",
+            vec!["host"],
             data_a,
             "sum(metric_a) by (host)",
             // second metric not used but the helper requires it; empty data.
             "dummy",
+            vec!["host"],
             vec![],
             "sum(dummy) by (host)",
         );
@@ -225,9 +235,11 @@ mod tests {
         let data_a = host_a_series([(1000, 0.9), (2000, 0.75)]);
         let engine = create_range_engine_two_metrics(
             "metric_a",
+            vec!["host"],
             data_a,
             "sum(metric_a) by (host)",
             "dummy",
+            vec!["host"],
             vec![],
             "sum(dummy) by (host)",
         );
@@ -243,4 +255,35 @@ mod tests {
         assert!((by_ts[&1000] - 0.1).abs() < 1e-10);
         assert!((by_ts[&2000] - 0.25).abs() < 1e-10);
     }
+
+    // Regression test: handle_binary_expr_range_promql's vector-vector join
+    // used to match purely on positional KeyByLabelValues equality (rhs
+    // labels discarded), unlike the instant-query combine_vector_vector,
+    // which rejects a join between arms grouped by different label sets. Two
+    // arms grouped by disjoint labels ((host) vs (region)) that happen to
+    // produce the same value could silently join into a wrong-but-plausible
+    // result across the whole range.
+    #[tokio::test(flavor = "multi_thread")]
+    async fn test_range_vector_vector_mismatched_label_sets_return_none() {
+        let data_a = host_a_series([(1000, 10.0), (2000, 15.0)]);
+        let data_b = host_a_series([(1000, 10.0), (2000, 15.0)]);
+        let engine = create_range_engine_two_metrics(
+            "metric_a",
+            vec!["host"],
+            data_a,
+            "sum(metric_a) by (host)",
+            "metric_b",
+            vec!["region"],
+            data_b,
+            "sum(metric_b) by (region)",
+        );
+
+        let query = "sum(metric_a) by (host) + sum(metric_b) by (region)";
+        let result = engine.handle_range_query_promql(query.to_string(), 1.0, 2.0, 1.0);
+        assert!(
+            result.is_none(),
+            "BUG: arms grouped by different label sets must not join, even when their \
+             values coincide, got {result:?}"
+        );
+    }
 }