From aaf40131d4a9e7a2de032eb02d5e0a0a50e886aa Mon Sep 17 00:00:00 2001 From: zz_y Date: Sun, 5 Jul 2026 18:44:56 -0600 Subject: [PATCH] feat(promql): lower unary negation as multiply-by-minus-one (#36) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `-expr` was rejected outright — the L2 PromQL path had no way to express "sign-flip every sample". Now that a scalar operand exists (#35), negation lowers as `expr * -1`: a `Mul` BinaryOp of the vector against `Scalar(-1)`, with no vector match. A constant-foldable operand (`-(10*1024*1024)`) collapses to a negated `Scalar` leaf instead. `Mul` is commutative, so operand order carries no hazard, and the L3 BinaryOp schema rule already takes the vector side — negation is label-preserving for free. Negation composes with everything: it lowers inside aggregate arguments (`sum(-m)`), binary ops (`a - -b`), set ops (`-a or -b`), and nests (`- -m`), because the aggregate/binary walkers already recurse through the shared `walk`. The five cases the old `unary_negation_is_rejected__GAP` test pinned as rejected now lower correctly; that test is replaced by `unary_negation_lowers_as_multiply_by_minus_one` plus constant-fold and double-negation coverage, two exact-tree e2e pins, and a row in the nesting-contract table in docs/promql-lowering.md. Closes #36 Co-Authored-By: Claude Fable 5 --- crates/e2e/tests/binary_op.rs | 32 ++++++ crates/frontend-promql/src/error.rs | 4 +- crates/frontend-promql/src/promql.rs | 31 +++--- .../tests/promql_conformance.rs | 101 +++++++++++++++--- docs/promql-lowering.md | 2 +- 5 files changed, 142 insertions(+), 28 deletions(-) diff --git a/crates/e2e/tests/binary_op.rs b/crates/e2e/tests/binary_op.rs index 00bb4174..f2b18603 100644 --- a/crates/e2e/tests/binary_op.rs +++ b/crates/e2e/tests/binary_op.rs @@ -226,3 +226,35 @@ fn q21_div_two_sum_by_job() { expected, ); } + +// #36 — unary negation lowers as `expr * -1`: a Mul BinaryOp of the vector +// against Scalar(-1), no vector match. The vector side keeps its schema. +#[test] +fn q36_unary_negation_is_multiply_by_minus_one() { + let expected = QueryExpr::BinaryOp { + op: BinaryOpKind::Arith(ArithOp::Mul), + lhs: Box::new(scan("some_metric", &[])), + rhs: Box::new(QueryExpr::Scalar(-1.0)), + vector_match: None, + }; + assert_eq!(lower("-some_metric"), expected); +} + +// #36 — negation nested inside an aggregate argument (issue #27 nesting): +// `sum(-m)` → Aggregate{Sum} over the `m * -1` BinaryOp. +#[test] +fn q36_sum_of_negation_nests() { + let expected = QueryExpr::Aggregate { + by: vec![].into(), + aggs: vec![AggIntent::Sum { col: None }], + output_names: vec!["".into()], + having: None, + child: Box::new(QueryExpr::BinaryOp { + op: BinaryOpKind::Arith(ArithOp::Mul), + lhs: Box::new(scan("node_cpu_seconds_total", &[])), + rhs: Box::new(QueryExpr::Scalar(-1.0)), + vector_match: None, + }), + }; + assert_eq!(lower("sum(-node_cpu_seconds_total)"), expected); +} diff --git a/crates/frontend-promql/src/error.rs b/crates/frontend-promql/src/error.rs index 56d2aa6b..aaea0897 100644 --- a/crates/frontend-promql/src/error.rs +++ b/crates/frontend-promql/src/error.rs @@ -16,8 +16,8 @@ pub enum PromqlError { UnsupportedFunction(String), /// A PromQL aggregation operator (`sum`, `topk`, …) not supported. UnsupportedAggregateOp(String), - /// A structural feature (offset / `@` / `without` / unary negation) not - /// supported in this version. + /// A structural feature (offset / `@` / `without`) not supported in this + /// version. UnsupportedFeature(String), /// A required function / aggregator argument was missing. MissingArgument(String), diff --git a/crates/frontend-promql/src/promql.rs b/crates/frontend-promql/src/promql.rs index e6b02ac5..6571a3cf 100644 --- a/crates/frontend-promql/src/promql.rs +++ b/crates/frontend-promql/src/promql.rs @@ -210,14 +210,21 @@ fn walk(expr: &Expr) -> Result { Expr::Binary(bin) => walk_binary(bin), Expr::Paren(p) => walk(&p.expr), // `UnaryExpr` is built only by negation (`Neg`); unary `+` is folded to - // identity and `-` to a negated `NumberLiteral`, so this always - // wraps a vector expression whose samples must be sign-flipped. The L2 - // PromQL path has no scalar/negate node to express that (there's no - // `-1 * x`, since `walk` rejects bare scalar operands), so reject it - // rather than silently dropping the sign and computing `+expr`. - Expr::Unary(_) => Err(LoweringError::UnsupportedFeature( - "unary negation (`-expr`): no negate/scalar node in the L2 PromQL path".into(), - )), + // identity and `-` to a negated `NumberLiteral`, so this wraps a + // sub-expression whose samples must be sign-flipped. Now that a scalar + // operand exists (#35), express it as `x * -1` — a constant-foldable + // operand (`-(10*1024)`) collapses to a negated `Scalar` leaf; anything + // else is a vector, sign-flipped by a `Mul` against `Scalar(-1)`. `Mul` + // is commutative, so operand order carries no hazard (#36). + Expr::Unary(u) => match num_expr(&u.expr) { + Ok(v) => Ok(L2::Scalar(-v)), + Err(_) => Ok(L2::BinaryOp { + op: BinaryOpKind::Arith(ArithOp::Mul), + lhs: Box::new(walk(&u.expr)?), + rhs: Box::new(L2::Scalar(-1.0)), + vector_match: None, + }), + }, Expr::Subquery(sq) => Ok(L2::PromQLSubquery { range: sq.range, resolution: sq.step, @@ -364,10 +371,10 @@ fn walk_aggregate(agg: &AggregateExpr) -> Result { // elsewhere (`sum(histogram_quantile(0.9, …))`). Lower it recursively with // the same `walk` used at the top level, then wrap it in the outer // aggregation. This is the path that lifts the old two-level limit to - // arbitrary function nesting (issue #27). If the inner expression is itself - // unsupported (e.g. unary negation), `walk` surfaces that error, so a - // genuinely unsupported query is still cleanly rejected rather than - // mislowered. + // arbitrary function nesting (issue #27) — a negated argument (`sum(-m)`) + // lowers here too (#36). If the inner expression is itself unsupported (e.g. + // an `offset` modifier), `walk` surfaces that error, so a genuinely + // unsupported query is still cleanly rejected rather than mislowered. let child = walk(&agg.expr)?; build_over_subtree(outer, keys, child) } diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index 21dfbfdb..088d9c94 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -129,6 +129,27 @@ fn has bool>(e: &QueryExpr, pred: F) -> bool { intents(e).iter().any(pred) } +/// Whether the tree contains a `Mul`-by-`Scalar(-1)` anywhere — the shape unary +/// negation lowers to (issue #36). +fn negates_via_scalar(e: &QueryExpr) -> bool { + let is_neg_one = |q: &QueryExpr| matches!(q, QueryExpr::Scalar(v) if (*v + 1.0).abs() < 1e-12); + match e { + QueryExpr::BinaryOp { op, lhs, rhs, .. } => { + (*op == BinaryOpKind::Arith(ArithOp::Mul) && (is_neg_one(lhs) || is_neg_one(rhs))) + || negates_via_scalar(lhs) + || negates_via_scalar(rhs) + } + QueryExpr::Aggregate { child, .. } + | QueryExpr::Sort { child, .. } + | QueryExpr::Limit { child, .. } + | QueryExpr::TimeRange { child, .. } + | QueryExpr::Subquery { child, .. } + | QueryExpr::Filter { child, .. } + | QueryExpr::Project { child, .. } => negates_via_scalar(child), + _ => false, + } +} + // ───────────────────────────────────────────────────────────────────────────── // A. Selectors & label matchers (basics §"Instant/Range Vector // Selectors"; selectors.test) @@ -509,19 +530,73 @@ fn vector_comparison_filters() { } #[test] -fn unary_negation_is_rejected__GAP() { - // SEMANTICS (PromQL): `-expr` flips the sign of every sample (and `-rate(…)` - // negates the rate). With no negate/scalar node in the L2 PromQL path we - // can't model that, so it's rejected rather than silently lowered as `+expr` - // (which would compute the wrong result). `-` folds into the - // literal at parse time and is caught by the bare-scalar rejection instead. - let _ = rejected("-rate(http_errors_total[5m])"); - let _ = rejected("-some_metric"); - let _ = rejected("-metric_a or -metric_b"); - // Negation nested inside a larger expression propagates the rejection, - // rather than lowering the rest with the inner sign silently dropped. - let _ = rejected("http_requests_total - -http_errors_total"); - let _ = rejected("sum(-node_cpu_seconds_total)"); +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 `Scalar(-1)`. These are + // the five cases the old `__GAP` test pinned as rejected. + for q in [ + "-rate(http_errors_total[5m])", + "-some_metric", + "-metric_a or -metric_b", + "http_requests_total - -http_errors_total", + "sum(-node_cpu_seconds_total)", + ] { + let qe = ok(q); + // A `Mul`-by-`-1` against a `Scalar(-1)` appears somewhere in every tree. + assert!(negates_via_scalar(&qe), "no `* -1` negation found in {q}: {qe:?}"); + } + + // `-some_metric` at the root: `Scan * Scalar(-1)`, schema follows the vector. + let QueryExpr::BinaryOp { op, lhs, rhs, vector_match } = &ok("-some_metric") else { + panic!("expected a BinaryOp for `-some_metric`"); + }; + assert_eq!(*op, BinaryOpKind::Arith(ArithOp::Mul)); + assert!(matches!(lhs.as_ref(), QueryExpr::Scan { .. }), "vector on the left"); + assert!( + matches!(rhs.as_ref(), QueryExpr::Scalar(v) if (*v + 1.0).abs() < 1e-12), + "negation multiplies by Scalar(-1), got {rhs:?}" + ); + assert!(vector_match.is_none(), "scalar negation carries no vector match"); + // Label-preserving: the schema is the vector operand's, unchanged. + let schema = ok("-some_metric").output_schema().unwrap(); + assert_eq!( + schema.columns.iter().map(|c| c.name.as_str()).collect::>(), + vec!["ts", "value"], + ); + + // `sum(-m)` — the negation lowers inside the aggregate argument (issue #27 + // nesting), so the outer node is the `Sum` aggregate over the `Mul`. + let QueryExpr::Aggregate { aggs, child, .. } = &ok("sum(-node_cpu_seconds_total)") else { + panic!("expected an outer Aggregate for `sum(-m)`"); + }; + assert!(matches!(aggs.as_slice(), [AggIntent::Sum { .. }])); + assert!(matches!(child.as_ref(), QueryExpr::BinaryOp { + op: BinaryOpKind::Arith(ArithOp::Mul), .. + })); +} + +#[test] +fn unary_negation_of_constant_folds_to_scalar() { + // `-(10*1024*1024)` — the operand is constant-foldable, so negation collapses + // to a single negated `Scalar` leaf (no `BinaryOp`), just like a bare literal. + assert!(matches!( + ok("-(10*1024*1024)"), + QueryExpr::Scalar(v) if (v + 10_485_760.0).abs() < 1e-6 + )); +} + +#[test] +fn double_unary_negation_nests() { + // `- -some_metric` — negation of a negation: `(m * -1) * -1`. Both levels + // lower; the value is unchanged but the structure is faithfully nested. + let QueryExpr::BinaryOp { op, lhs, .. } = &ok("- -some_metric") else { + panic!("expected outer BinaryOp for `- -some_metric`"); + }; + assert_eq!(*op, BinaryOpKind::Arith(ArithOp::Mul)); + assert!(matches!(lhs.as_ref(), QueryExpr::BinaryOp { + op: BinaryOpKind::Arith(ArithOp::Mul), .. + }), "inner negation nests under the outer one"); } #[test] diff --git a/docs/promql-lowering.md b/docs/promql-lowering.md index f5508d9a..5ce91ed8 100644 --- a/docs/promql-lowering.md +++ b/docs/promql-lowering.md @@ -172,13 +172,13 @@ mislowered) — each row is pinned by a named test. | Range function over a sub-query (#42) | `max_over_time(rate(m[5m])[1h:])` | `over_time_of_subquery_reduces_per_series` | | Nested sub-queries (range fn over range fn over range fn; default resolution) | `max_over_time(deriv(rate(distance_covered_total[5s])[30s:5s])[10m:])` (Prometheus docs example) | `nested_subquery_from_prometheus_docs`; e2e `q27_nested_subquery_prometheus_docs_example` | | Outer group key absent from a nested aggregate's (closed) output (#53) | `sum(sum by (k)(m)) by (j)` — `j` provably absent → dropped per PromQL's absent-label grouping semantics | `outer_group_key_absent_from_nested_aggregate_is_dropped`; e2e `q53_outer_group_key_absent_from_nested_aggregate` | +| Unary negation, anywhere in a nest (#36) | `sum(-m)` → `expr * -1` (`Mul` against `Scalar(-1)`); `-(const)` folds to a negated `Scalar` | `unary_negation_lowers_as_multiply_by_minus_one`; e2e `q36_sum_of_negation_nests` | | SQL derived tables / inline views (#29) | `SELECT … FROM (SELECT … GROUP BY …) t`, incl. aggregate-over-aggregate | `derived_table_aggregate_over_aggregate_nests` (`sql_lowering.rs`) | ### Rejected cleanly | Shape | Why | Pinned by | |---|---|---| -| Unary negation anywhere in a nest — `sum(-m)` | no scalar-negate in the L2 PromQL path yet (issue #36) | `unary_negation_is_rejected__GAP` | | `without(...)` grouping | a usage-derived (open) schema can't enumerate the label complement (issue #39) | binder rejection (see "Why the Binder is its own pass") | | SQL subquery-valued **predicate** expressions — scalar `x > (SELECT …)`, `IN (SELECT …)`, `EXISTS` / `NOT EXISTS` / `NOT IN`, correlated or not | the v1 decision on #27's open question: these need a subquery node in the L2 expression IR + a correlated-vs-uncorrelated representation choice; rejected until that lands (derived tables in `FROM` are the supported nesting shape) | `scalar_subquery_in_predicate_is_rejected`, `semi_join_is_rejected_not_mislowered`, `exists_subquery_in_predicate_is_rejected` (`sql_lowering.rs`) |