From 0a4c49c47b0437944bd2a9536c6ced60dc471e2a Mon Sep 17 00:00:00 2001 From: Milind Srivastava Date: Fri, 30 Oct 2026 19:06:23 -0400 Subject: [PATCH] fix(frontend-metricsql): lower vector negation as PromQL does (#1096) MetricsQL lowered `-x` as `x * -1` (a PromqlScalarOp Mul), while the PromQL front end lowers it as a PromqlMap whose sample is `Negative` and which drops the metric name. The two front ends therefore produced different IR for the same query, and the comment claiming they matched was false. The regression test lowers `-rate(up[5m])` through both front ends and asserts equal query roots. Stale `* -1` test names and comments on the PromQL side are corrected; their assertions already checked `Negative`. Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + crates/frontend-metricsql/Cargo.toml | 1 + crates/frontend-metricsql/src/lib.rs | 14 ++++++++------ .../tests/lowering_semantics.rs | 13 +++++++++++++ .../tests/promql_conformance.rs | 19 ++++++++----------- crates/integration-tests/tests/binary_op.rs | 9 ++++----- 6 files changed, 35 insertions(+), 22 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ee46223cc..d63fe75f3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -359,6 +359,7 @@ version = "0.1.0" dependencies = [ "asap-frontend-common", "asap-frontend-metricsql", + "asap-frontend-promql", "asap-types", "metricsql_parser", "thiserror 2.0.18", diff --git a/crates/frontend-metricsql/Cargo.toml b/crates/frontend-metricsql/Cargo.toml index 912119261..69f5bf8a5 100644 --- a/crates/frontend-metricsql/Cargo.toml +++ b/crates/frontend-metricsql/Cargo.toml @@ -22,3 +22,4 @@ path = "tests/main.rs" [dev-dependencies] asap-types = { path = "../types", features = ["test-support"] } asap-frontend-metricsql = { path = ".", features = ["test-support"] } +asap-frontend-promql = { path = "../frontend-promql" } diff --git a/crates/frontend-metricsql/src/lib.rs b/crates/frontend-metricsql/src/lib.rs index d4c81d27b..4a3172506 100644 --- a/crates/frontend-metricsql/src/lib.rs +++ b/crates/frontend-metricsql/src/lib.rs @@ -127,13 +127,15 @@ impl Lowerer<'_> { Expr::NumberLiteral(_) => { Err(unsupported("scalar root requires lower_metricsql_query")) } - // Vector negation is `x * -1` (as in the PromQL front end). - Expr::UnaryOperator(e) => Ok(U::PromqlScalarOp { + // Vector negation negates each sample and drops the metric name, + // lowered exactly as the PromQL front end does. + Expr::UnaryOperator(e) => Ok(U::PromqlMap { child: Rc::new(self.lower(&e.expr)?), - scalar: UnresolvedScalar::Literal(ScalarValue::Float64(-1.0)), - op: BinaryOpKind::Arithmetic(ArithmeticOpKind::Mul), - scalar_left: false, - return_bool: false, + sample: UnresolvedScalar::Negative { + expr: Box::new(UnresolvedScalar::Column(ColumnRef::SampleValue)), + semantics: ExprSemantics::Promql, + }, + drop_metric_name: true, }), Expr::BinaryOperator(e) => self.binary(e), Expr::Parens(e) if e.expressions.len() == 1 => self.lower(&e.expressions[0]), diff --git a/crates/frontend-metricsql/tests/lowering_semantics.rs b/crates/frontend-metricsql/tests/lowering_semantics.rs index a46540441..3a1eb7842 100644 --- a/crates/frontend-metricsql/tests/lowering_semantics.rs +++ b/crates/frontend-metricsql/tests/lowering_semantics.rs @@ -148,3 +148,16 @@ fn workload_lowering_uses_the_declared_sample_interval() { }); lower_metricsql_query_workload(&at_rest).unwrap(); } + +/// Vector negation lowers to the same IR as PromQL's: a sample-value +/// `Negative` that drops the metric name, not a multiplication by -1. +#[test] +fn vector_negation_lowers_as_in_promql() { + let query = "-rate(up[5m])"; + let metricsql = + lower_metricsql_query_workload(&workload(QueryLanguage::MetricsQL, query)).unwrap(); + let promql = + asap_frontend_promql::lower_promql_query_workload(&workload(QueryLanguage::PromQL, query)) + .unwrap(); + assert_eq!(metricsql, promql); +} diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index 41376b185..a20d00f0d 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -112,7 +112,7 @@ fn has bool>(e: &OperatorNode, pred: F) -> bool { intents(e).iter().any(pred) } -/// Whether the tree contains a `Mul`-by-`ScalarExpr(-1)` anywhere — the shape unary +/// Whether the tree contains a `ScalarExpr::Negative` anywhere — the shape unary /// negation lowers to (issue #36). fn negates_via_scalar(e: &OperatorNode) -> bool { fn negative(expr: &ScalarExpr) -> bool { @@ -538,11 +538,11 @@ fn comparison_bool_modifier_returns_zero_or_one() { } #[test] -fn unary_negation_lowers_as_multiply_by_minus_one() { - // SEMANTICS (PromQL, issue #36): `-expr` flips the sign of every sample. - // Now that a scalar operand exists (#35), it lowers as `expr * -1` — a `Mul` - // BinaryOp of the (label-preserving) vector against `ScalarExpr(-1)`. These are - // the five cases the old `__GAP` test pinned as rejected. +fn unary_negation_lowers_as_a_negative_projection() { + // SEMANTICS (PromQL, issue #36): `-expr` flips the sign of every sample and + // drops the metric name. It lowers as a projection whose value column is + // `ScalarExpr::Negative`. These are the five cases the old `__GAP` test + // pinned as rejected. for q in [ "-rate(http_errors_total[5m])", "-some_metric", @@ -551,11 +551,8 @@ fn unary_negation_lowers_as_multiply_by_minus_one() { "sum(-node_cpu_seconds_total)", ] { let qe = ok(q); - // A `Mul`-by-`-1` against a `ScalarExpr(-1)` appears somewhere in every tree. - assert!( - negates_via_scalar(&qe), - "no `* -1` negation found in {q}: {qe:?}" - ); + // A `Negative` appears somewhere in every tree. + assert!(negates_via_scalar(&qe), "no negation found in {q}: {qe:?}"); } let negated = ok("-some_metric"); diff --git a/crates/integration-tests/tests/binary_op.rs b/crates/integration-tests/tests/binary_op.rs index a98f187bb..13153538f 100644 --- a/crates/integration-tests/tests/binary_op.rs +++ b/crates/integration-tests/tests/binary_op.rs @@ -268,11 +268,10 @@ fn q21_div_two_sum_by_job() { ); } -// #36 — unary negation lowers as `expr * -1`: a Mul BinaryOp of the vector -// against a `ScalarExpr(-1)` leaf, no vector match. The vector side keeps -// its schema. +// #36 — unary negation lowers as a projection whose value is +// `ScalarExpr::Negative`, no vector match. The vector side keeps its schema. #[test] -fn q36_unary_negation_is_multiply_by_minus_one() { +fn q36_unary_negation_is_a_negative_projection() { let root = lower("-some_metric"); let NonASAPOp::Project { cols, child, .. } = root.expect_non_asap() else { panic!() @@ -282,7 +281,7 @@ fn q36_unary_negation_is_multiply_by_minus_one() { } // #36 — negation nested inside an aggregate argument (issue #27 nesting): -// `sum(-m)` → Aggregate{Sum} over the `m * -1` BinaryOp. +// `sum(-m)` → Aggregate{Sum} over the negating projection. #[test] fn q36_sum_of_negation_nests() { let root = lower("sum(-node_cpu_seconds_total)");