From 703858b6c424a2cc52aa985c47ccec9d6dcdd51c Mon Sep 17 00:00:00 2001 From: zz_y Date: Thu, 2 Jul 2026 16:38:57 -0600 Subject: [PATCH] =?UTF-8?q?feat(promql):=20scalar=20operand=20for=20Binary?= =?UTF-8?q?Op=20=E2=80=94=20vector-op-scalar=20lowering=20(#35)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ` op ` — threshold alerts (`v > 10*1024*1024`), unit conversions (`rate(m[5m]) * 100`) — did not lower: BinaryOp required both operands to be vectors and the front end rejected bare number literals. This was the single biggest real-world blocker (~822/949 of the alerts corpus). Adds a `Scalar(f64)` leaf to the L2 relational tree and the L3 canonical IR. A number literal (and a constant-folded scalar expression like `10*1024*1024` / `24 * 3600`) lowers to `Scalar`; a `BinaryOp` operand folds to a scalar when it has no vector selector, else walks as a vector. The BinaryOp output schema follows the vector side (a scalar contributes no labels); `num_expr` now constant-folds arithmetic (also lets `topk(2+1, …)` work). Impact: awesome-prometheus-alerts corpus coverage jumps from 13 to 863/949 lowered. Flipped `scalar_literal_operand_is_rejected__GAP` and `scalar_threshold_comparisons_are_rejected__GAP` into passing tests; raised the corpus ratchet 12 -> 800. Vector-vector BinaryOp and all other paths unchanged. Full workspace suite green; clippy --all-targets clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/frontend-promql/src/promql.rs | 48 +++++++++++++++++-- .../tests/awesome_prometheus_alerts.rs | 25 +++++----- .../tests/promql_conformance.rs | 35 +++++++++++--- .../tests/synthetic_packet_trace.rs | 2 +- crates/ir/src/intent_algebra/query_expr.rs | 22 ++++++++- crates/l2/src/lower.rs | 2 + crates/l2/src/relational.rs | 8 +++- 7 files changed, 116 insertions(+), 26 deletions(-) diff --git a/crates/frontend-promql/src/promql.rs b/crates/frontend-promql/src/promql.rs index 743f4001..7028e202 100644 --- a/crates/frontend-promql/src/promql.rs +++ b/crates/frontend-promql/src/promql.rs @@ -192,8 +192,12 @@ fn walk(expr: &Expr) -> Result { input: Box::new(filtered_source(metric, matchers)), }) } - Expr::NumberLiteral(_) | Expr::StringLiteral(_) => Err(LoweringError::UnsupportedFeature( - "bare scalar/string at top level".into(), + // A number literal is a scalar leaf (`v > 5`, or a bare scalar query + // `5`). String literals only appear as function args (`label_replace`, + // …), which are not supported, so reject them (issue #35). + Expr::NumberLiteral(n) => Ok(L2::Scalar(n.val)), + Expr::StringLiteral(_) => Err(LoweringError::UnsupportedFeature( + "bare string literal".into(), )), Expr::Extension(_) => Err(LoweringError::UnsupportedFeature( "extension expression".into(), @@ -413,8 +417,8 @@ fn walk_histogram_quantile(call: &Call) -> Result { } fn walk_binary(bin: &BinaryExpr) -> Result { - let lhs = walk(&bin.lhs)?; - let rhs = walk(&bin.rhs)?; + let lhs = scalar_or_vector(&bin.lhs)?; + let rhs = scalar_or_vector(&bin.rhs)?; let op = binop(bin.op.id())?; let vector_match = bin.modifier.as_ref().map(|m| { let (kind, labels) = match &m.matching { @@ -881,13 +885,47 @@ fn num_param(agg: &AggregateExpr) -> Result { fn num_expr(expr: &Expr) -> Result { match expr { Expr::NumberLiteral(n) => Ok(n.val), + Expr::Paren(p) => num_expr(&p.expr), + // Constant-fold a pure scalar arithmetic expression — the parser does + // not fold `10*1024*1024` / `24 * 3600`. A `modifier` (vector matching) + // or a non-arithmetic operator means it is not a pure scalar. + Expr::Binary(b) if b.modifier.is_none() => { + let (l, r) = (num_expr(&b.lhs)?, num_expr(&b.rhs)?); + let id = b.op.id(); + if id == token::T_ADD { + Ok(l + r) + } else if id == token::T_SUB { + Ok(l - r) + } else if id == token::T_MUL { + Ok(l * r) + } else if id == token::T_DIV { + Ok(l / r) + } else if id == token::T_MOD { + Ok(l % r) + } else if id == token::T_POW { + Ok(l.powf(r)) + } else { + Err(LoweringError::InvalidParameter( + "non-arithmetic operator in scalar expression".into(), + )) + } + } other => Err(LoweringError::InvalidParameter(format!( - "expected a numeric literal, got {:?}", + "expected a numeric scalar, got {:?}", std::mem::discriminant(other) ))), } } +/// A `BinaryOp` operand: fold a pure-scalar expression (`5`, `10*1024*1024`) to +/// a `Scalar` leaf, otherwise walk it as a vector (issue #35). +fn scalar_or_vector(expr: &Expr) -> Result { + match num_expr(expr) { + Ok(v) => Ok(L2::Scalar(v)), + Err(_) => walk(expr), + } +} + /// `topk`/`bottomk` count parameter — a non-negative integer. Rejects /// fractional / negative / non-finite values rather than silently truncating /// or saturating them via `as u64` (`topk(2.7, …)` ≠ `topk(2, …)`). diff --git a/crates/frontend-promql/tests/awesome_prometheus_alerts.rs b/crates/frontend-promql/tests/awesome_prometheus_alerts.rs index c1610c0b..97d58f8d 100644 --- a/crates/frontend-promql/tests/awesome_prometheus_alerts.rs +++ b/crates/frontend-promql/tests/awesome_prometheus_alerts.rs @@ -91,7 +91,7 @@ fn intents(e: &QueryExpr) -> Vec { go(expr, out); go(child, out); } - QueryExpr::Scan { .. } | QueryExpr::Ref { .. } => {} + QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::Ref { .. } => {} } } go(e, &mut out); @@ -143,11 +143,12 @@ fn corpus_lowering_is_total_and_fully_parseable() { t.unparseable ); - // Coverage floor (ratchet, not an exact count): at minimum the - // vector-vs-vector comparisons lower. Lifting the scalar-threshold or - // nested-function gaps should raise this deliberately. + // Coverage floor (ratchet, not an exact count). The scalar-threshold operand + // (#35) unblocked the dominant ` ` shape, taking + // coverage from ~13 to ~863/949. Remaining gaps are un-implemented functions + // (histogram_*, absent, changes-derivatives-with-scalar, without(), …). assert!( - t.lowered >= 12, + t.lowered >= 800, "real-world lowering coverage regressed: {t:?}" ); } @@ -238,19 +239,21 @@ fn all_targets_missing_core_lowers() { // ───────────────────────────────────────────────────────────────────────────── #[test] -fn scalar_threshold_comparisons_are_rejected__GAP() { +fn scalar_threshold_comparisons_lower_to_binaryop_scalar() { // ~822/949 corpus queries are ` `. The numeric - // threshold operand has no L2 scalar node, so the whole alert is rejected - // (cleanly) — this is the single biggest blocker to lowering real alerts. - // All three reject via the bare-scalar-operand path (UnsupportedFeature). + // threshold is now a `Scalar` operand of the `BinaryOp` (issue #35) — the + // single biggest unblock for real alerts. for q in [ "prometheus_config_last_reload_successful != 1", "increase(prometheus_tsdb_compactions_failed_total[1m]) > 0", "rate(alertmanager_notifications_failed_total[3m]) > 0.05", ] { + let QueryExpr::BinaryOp { rhs, .. } = ok(q) else { + panic!("expected a BinaryOp for {q:?}"); + }; assert!( - matches!(rejected(q), LoweringError::UnsupportedFeature(_)), - "expected a clean UnsupportedFeature rejection for {q:?}" + matches!(rhs.as_ref(), QueryExpr::Scalar(_)), + "scalar threshold operand for {q:?}, got {rhs:?}" ); } } diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index fe7aec19..4000415a 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -91,7 +91,7 @@ fn collect(e: &QueryExpr, out: &mut Vec) { collect(expr, out); collect(child, out); } - QueryExpr::Scan { .. } | QueryExpr::Ref { .. } => {} + QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::Ref { .. } => {} } } @@ -546,11 +546,34 @@ fn count_maps_to_cardinality_and_inherits_accuracy() { } #[test] -fn scalar_literal_operand_is_rejected__GAP() { - // SEMANTICS (PromQL): `v > 10*1024*1024` filters by a scalar threshold. - // We have no scalar/number-literal expression in L2, so a literal operand - // is rejected. Common real-world thresholds therefore don't lower yet. - let _ = rejected("node_filesystem_avail_bytes > 10*1024*1024"); +fn scalar_literal_operand_lowers_as_binaryop_scalar() { + // Issue #35: ` op ` — the numeric threshold is a `Scalar` + // operand of the `BinaryOp`, and constant arithmetic (`10*1024*1024`) is + // folded. The output schema is the vector side's. + let qe = ok("node_filesystem_avail_bytes > 10*1024*1024"); + let QueryExpr::BinaryOp { op, lhs, rhs, .. } = &qe else { + panic!("expected a BinaryOp, got {qe:?}"); + }; + assert_eq!(*op, BinaryOpKind::Compare(CompareOp::Gt)); + assert!(matches!(lhs.as_ref(), QueryExpr::Scan { .. }), "vector on the left"); + assert!( + matches!(rhs.as_ref(), QueryExpr::Scalar(v) if (*v - 10_485_760.0).abs() < 1e-6), + "folded scalar threshold on the right, got {rhs:?}" + ); + // Schema derivation follows the vector side (a scalar contributes no labels). + assert!(qe.output_schema().is_ok()); +} + +#[test] +fn scalar_arithmetic_scales_the_vector() { + // `rate(m[5m]) * 100` — a unit conversion. Arithmetic BinaryOp of the vector + // with a `Scalar(100)`. + let qe = ok("rate(m[5m]) * 100"); + let QueryExpr::BinaryOp { op, rhs, .. } = &qe else { + panic!("expected a BinaryOp, got {qe:?}"); + }; + assert_eq!(*op, BinaryOpKind::Arith(ArithOp::Mul)); + assert!(matches!(rhs.as_ref(), QueryExpr::Scalar(v) if (*v - 100.0).abs() < 1e-9)); } // ───────────────────────────────────────────────────────────────────────────── diff --git a/crates/frontend-sql/tests/synthetic_packet_trace.rs b/crates/frontend-sql/tests/synthetic_packet_trace.rs index 6556289e..80d12b47 100644 --- a/crates/frontend-sql/tests/synthetic_packet_trace.rs +++ b/crates/frontend-sql/tests/synthetic_packet_trace.rs @@ -97,7 +97,7 @@ fn intents(e: &QueryExpr) -> Vec { go(expr, out); go(child, out); } - QueryExpr::Scan { .. } | QueryExpr::Ref { .. } => {} + QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::Ref { .. } => {} } } go(e, &mut out); diff --git a/crates/ir/src/intent_algebra/query_expr.rs b/crates/ir/src/intent_algebra/query_expr.rs index 4ba46f74..a324edc0 100644 --- a/crates/ir/src/intent_algebra/query_expr.rs +++ b/crates/ir/src/intent_algebra/query_expr.rs @@ -264,6 +264,11 @@ pub enum QueryExpr { /// Reference to a `LetBinding` by name; resolved at plan time. Ref { name: BindingName }, + /// A scalar constant leaf — a PromQL number literal or a folded constant + /// scalar expression (`10*1024*1024`). Appears as a [`BinaryOp`](Self::BinaryOp) + /// operand for ` op ` thresholds / unit conversions (#35). + Scalar(f64), + /// σ — row-level filter. Output schema = child schema. Filter { pred: Predicate, @@ -642,7 +647,22 @@ impl QueryExpr { Ok(out) } - QueryExpr::BinaryOp { lhs, .. } => lhs.output_schema_in(scope), + // A scalar constant has no series — model it as a single `value` + // column so it can sit as a `BinaryOp` operand. + QueryExpr::Scalar(_) => Ok(Schema { + columns: vec![Column::new("value", DataType::Float64, false)], + time_index: None, + unique_keys: Vec::new(), + closed: true, + }), + + // The output shape of ` op ` (or ` op + // `) is the vector side's — a scalar operand contributes only + // its value, no labels. Prefer the non-`Scalar` side. + QueryExpr::BinaryOp { lhs, rhs, .. } => match (lhs.as_ref(), rhs.as_ref()) { + (QueryExpr::Scalar(_), r) => r.output_schema_in(scope), + (l, _) => l.output_schema_in(scope), + }, } } } diff --git a/crates/l2/src/lower.rs b/crates/l2/src/lower.rs index 9c28c993..54f6b39e 100644 --- a/crates/l2/src/lower.rs +++ b/crates/l2/src/lower.rs @@ -83,6 +83,8 @@ pub fn convert( Ok(match legacy { LQueryExpr::Source(spec) => scan(spec, fallback, &[])?, + LQueryExpr::Scalar(v) => CQueryExpr::Scalar(*v), + LQueryExpr::Ref(name) => CQueryExpr::Ref { name: BindingName::new(name.clone()), }, diff --git a/crates/l2/src/relational.rs b/crates/l2/src/relational.rs index 9e1fc33b..8823040d 100644 --- a/crates/l2/src/relational.rs +++ b/crates/l2/src/relational.rs @@ -133,6 +133,10 @@ pub enum AggFunc { pub enum QueryExpr { /// A named metric stream or table — the outermost leaf. Source(SourceSpec), + /// A scalar constant leaf — a PromQL number literal (or a folded constant + /// scalar expression like `10*1024*1024`). Appears as a `BinaryOp` operand + /// for ` op ` thresholds / unit conversions (issue #35). + Scalar(f64), /// Reference to a CTE / let-binding by name. **Reserved**: no front end /// emits `Ref`/`LetBinding` yet (CSE runs on L3); the converter arm exists /// for forward-compatibility (e.g. PromQL recording rules). @@ -254,7 +258,7 @@ impl QueryExpr { pub fn walk(&self, f: &mut F) { f(self); match self { - QueryExpr::Source(_) | QueryExpr::Ref(_) => {} + QueryExpr::Source(_) | QueryExpr::Scalar(_) | QueryExpr::Ref(_) => {} QueryExpr::Filter { input, .. } | QueryExpr::Project { input, .. } | QueryExpr::Aggregate { input, .. } @@ -306,7 +310,7 @@ impl QueryExpr { | QueryExpr::SetOp { left, .. } | QueryExpr::BinaryOp { lhs: left, .. } => left.leaf_source(), QueryExpr::LetBinding { body, .. } => body.leaf_source(), - QueryExpr::Ref(_) => None, + QueryExpr::Scalar(_) | QueryExpr::Ref(_) => None, } }