From 10ec36b99722b54a5b1059091175a6380bdae24f Mon Sep 17 00:00:00 2001 From: zz_y Date: Sat, 22 Aug 2026 16:36:38 -0600 Subject: [PATCH] refactor(types): collapse QueryExpr::PromqlScalar into Literal (#220) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instance 1 of #220: `QueryExpr::PromqlScalar(f64)` held exactly the same value `QueryExpr::Literal(ScalarValue::Float64(_))` does, just at a different tree position (a `BinaryOp` operand / query root, vs. a `Compare`/`Arithmetic` operand). Collapses the two into one variant, `Literal`, plus a new marker wrapper `PromqlScalarBridge(Rc>)` that carries the tree-position distinction the old variant tag used to: wrapping a `Literal` in `PromqlScalarBridge` is what now says "this scalar sub-expression sits at an operator-tree position and has its own row schema" (`output_schema`'s `BinaryOp`/root arms), vs. a bare `Literal` in a scalar-sub-language position, which still has none (`QueryExprError::ScalarHasNoRowSchema`). - `query_expr.rs`: removes `PromqlScalar`, adds `PromqlScalarBridge`, and the `promql_scalar`/`as_promql_scalar` constructor/accessor every call site now uses in place of the old bare variant. - `canonicalize.rs`, `resolve.rs`, `binder.rs`: the bridge's child is a genuine scalar-sub-language node now, routed through `resolve_expr`/ `columns_referenced` like every other scalar position, instead of being an opaque `f64` leaf. - `cse.rs`, `dag_export.rs`: minimal surgical updates (kept out of scope otherwise per the CSE work happening elsewhere) — the bridge is still treated as a single opaque unit, matching the old `PromqlScalar` leaf's treatment exactly. - Both front ends' test suites, `devtools`, `integration-tests`: every call site updated; behavior preserved exactly (all existing tests pass unchanged). Adds unit tests in `query_expr.rs` and `resolve.rs` pinning: the bridge/bare-literal value equivalence, that the row-schema/no-row- schema split now rides on the wrapper rather than the variant, and that `BinaryOp`'s `vector_match` semantics are unaffected. Instance 2 (`BinaryOp` vs `Compare`/`Arithmetic`) is scoped out as a separate, larger change — tracked in #245. Co-Authored-By: Claude Sonnet 5 --- .../devtools/examples/canonical_examples.rs | 2 +- crates/devtools/src/bin/variant_coverage.rs | 6 +- crates/frontend-promql/src/promql.rs | 24 +-- .../awesome_prometheus_alerts.rs | 12 +- .../tests/promql_conformance.rs | 76 +++++---- .../synthetic_packet_trace.rs | 4 +- crates/frontend-sql/tests/netflow/netflow.rs | 2 +- crates/integration-tests/tests/binary_op.rs | 6 +- crates/types/src/dag_export.rs | 13 +- crates/types/src/pre_asap/binder.rs | 13 +- crates/types/src/pre_asap/canonicalize.rs | 5 +- crates/types/src/pre_asap/cse.rs | 9 +- crates/types/src/pre_asap/query_expr.rs | 156 ++++++++++++++++-- crates/types/src/pre_asap/resolve.rs | 69 +++++++- docs/pre-asap-ir.md | 14 +- 15 files changed, 326 insertions(+), 85 deletions(-) diff --git a/crates/devtools/examples/canonical_examples.rs b/crates/devtools/examples/canonical_examples.rs index dd1250f3..8fda4b58 100644 --- a/crates/devtools/examples/canonical_examples.rs +++ b/crates/devtools/examples/canonical_examples.rs @@ -48,7 +48,7 @@ fn bgp_catalog() -> SqlCatalog { async fn main() { let promql_examples: &[(&str, &str)] = &[ ("Scan", "up"), - ("BinaryOp + PromqlScalar", "up > 1"), + ("BinaryOp + PromqlScalarBridge", "up > 1"), ("QueryTimestamp", "time()"), ("Aggregate", "sum(up)"), ( diff --git a/crates/devtools/src/bin/variant_coverage.rs b/crates/devtools/src/bin/variant_coverage.rs index 168a74c2..6d5d08e8 100644 --- a/crates/devtools/src/bin/variant_coverage.rs +++ b/crates/devtools/src/bin/variant_coverage.rs @@ -14,7 +14,7 @@ use std::collections::BTreeSet; const ALL_VARIANTS: &[&str] = &[ "Scan", - "PromqlScalar", + "PromqlScalarBridge", "QueryTimestamp", "PromqlVectorFromScalar", "PromqlScalarFromVector", @@ -42,8 +42,8 @@ fn walk(e: &QueryExpr, seen: &mut BTreeSet<&'static str>) { QueryExpr::Scan { .. } => { seen.insert("Scan"); } - QueryExpr::PromqlScalar(_) => { - seen.insert("PromqlScalar"); + QueryExpr::PromqlScalarBridge(_) => { + seen.insert("PromqlScalarBridge"); } QueryExpr::QueryTimestamp => { seen.insert("QueryTimestamp"); diff --git a/crates/frontend-promql/src/promql.rs b/crates/frontend-promql/src/promql.rs index 5eb6a4f2..3d8cb174 100644 --- a/crates/frontend-promql/src/promql.rs +++ b/crates/frontend-promql/src/promql.rs @@ -38,7 +38,7 @@ //! | `increase(m[w])` | `Aggregate{[Increase], TimeRange{w}}` | //! | `changes`/`delta`/`idelta`/`deriv`/`resets`/`predict_linear`/`double_exponential_smoothing`(`m[w]`, …) | `Aggregate{[Changes/Delta/…], TimeRange{w}}` — per-series counter-derivative intents (issue #44) | //! | `absent(v)` / `absent_over_time(m[w])` / `present_over_time(m[w])` | `Aggregate{[Absent/AbsentOverTime/PresentOverTime]}` — presence intents; the empty→synthesized-sample logic is a post-ASAP concern (issue #47) | -//! | `abs`/`ceil`/`sqrt`/`ln`/`clamp*`/`round`/trig(`v`), `pi()` | `Aggregate{[Math(f)]}` element-wise transform (issue #45); `pi()` → a `PromqlScalar` leaf | +//! | `abs`/`ceil`/`sqrt`/`ln`/`clamp*`/`round`/trig(`v`), `pi()` | `Aggregate{[Math(f)]}` element-wise transform (issue #45); `pi()` → a `PromqlScalarBridge` leaf | //! | `time()` / `timestamp`/`hour`/`day_of_week`/… (`v`) | `QueryTimestamp` leaf / `Aggregate{[TimeFn(f)]}` (issue #46) | //! | `vector(s)` / `scalar(v)` | `PromqlVectorFromScalar` / `PromqlScalarFromVector` — the scalar⇄vector bridges (issue #48) | //! | `label_replace(v,…)` / `label_join(v,…)` | `PromqlRelabel{dst, value}` — per-series label rewrite; value unchanged (issue #50) | @@ -264,10 +264,10 @@ fn walk(expr: &Expr) -> Result { Expr::Call(call) if is_typeconv_fn(call.func.name) => walk_typeconv(call), Expr::Call(call) if is_label_fn(call.func.name) => walk_label(call), Expr::Call(call) if is_sort_fn(call.func.name) => walk_sort(call), - // A bare `min_of`/`max_of(consts…)` scalar query folds to a `PromqlScalar` + // A bare `min_of`/`max_of(consts…)` scalar query folds to a `PromqlScalarBridge` // leaf; a non-constant argument makes `num_expr` fail → rejected (#89). Expr::Call(call) if is_scalar_reducer_fn(call.func.name) => { - Ok(Unresolved::PromqlScalar(num_expr(expr)?)) + Ok(Unresolved::promql_scalar(num_expr(expr)?)) } Expr::Call(call) if call.func.name == "info" => walk_info(call), Expr::Call(call) => walk_call(call), @@ -277,15 +277,15 @@ fn walk(expr: &Expr) -> Result { // 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 `PromqlScalar` leaf; anything - // else is a vector, sign-flipped by a `Mul` against `PromqlScalar(-1)`. `Mul` + // operand (`-(10*1024)`) collapses to a negated `PromqlScalarBridge` leaf; anything + // else is a vector, sign-flipped by a `Mul` against `PromqlScalarBridge(-1)`. `Mul` // is commutative, so operand order carries no hazard (#36). Expr::Unary(u) => match num_expr(&u.expr) { - Ok(v) => Ok(Unresolved::PromqlScalar(-v)), + Ok(v) => Ok(Unresolved::promql_scalar(-v)), Err(_) => Ok(Unresolved::BinaryOp { op: BinaryOpKind::Arithmetic(ArithmeticOpKind::Mul), lhs: Rc::new(walk(&u.expr)?), - rhs: Rc::new(Unresolved::PromqlScalar(-1.0)), + rhs: Rc::new(Unresolved::promql_scalar(-1.0)), vector_match: None, }), }, @@ -308,7 +308,7 @@ fn walk(expr: &Expr) -> Result { // 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(Unresolved::PromqlScalar(n.val)), + Expr::NumberLiteral(n) => Ok(Unresolved::promql_scalar(n.val)), Expr::StringLiteral(_) => Err(LoweringError::UnsupportedFeature( "bare string literal".into(), )), @@ -1061,10 +1061,10 @@ fn is_math_fn(name: &str) -> bool { /// A math / trig function — a per-series element-wise value transform, lowered /// to a per-series `Aggregate{[Math(f)]}` over the (instant) argument vector. -/// `pi()` is the constant π, lowered to a `PromqlScalar` leaf (issue #45). +/// `pi()` is the constant π, lowered to a `PromqlScalarBridge` leaf (issue #45). fn walk_math(call: &Call) -> Result { if call.func.name == "pi" { - return Ok(Unresolved::PromqlScalar(std::f64::consts::PI)); + return Ok(Unresolved::promql_scalar(std::f64::consts::PI)); } let func = match call.func.name { "abs" => MathFunc::Abs, @@ -1921,10 +1921,10 @@ fn is_scalar_reducer_fn(name: &str) -> bool { } /// A `BinaryOp` operand: fold a pure-scalar expression (`5`, `10*1024*1024`) to -/// a `PromqlScalar` leaf, otherwise walk it as a vector (issue #35). +/// a `PromqlScalarBridge` leaf, otherwise walk it as a vector (issue #35). fn scalar_or_vector(expr: &Expr) -> Result { match num_expr(expr) { - Ok(v) => Ok(Unresolved::PromqlScalar(v)), + Ok(v) => Ok(Unresolved::promql_scalar(v)), Err(_) => walk(expr), } } diff --git a/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs b/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs index 2592a62c..9c0c99db 100644 --- a/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs +++ b/crates/frontend-promql/tests/observability/awesome_prometheus_alerts.rs @@ -89,7 +89,9 @@ fn intents(e: &QueryExpr) -> Vec { } // `AggIntent` only ever lives in `Aggregate.measures`, never in a // scalar position (issue #205) — nothing to collect there. - QueryExpr::Scan { .. } | QueryExpr::PromqlScalar(_) | QueryExpr::QueryTimestamp => {} + QueryExpr::Scan { .. } + | QueryExpr::PromqlScalarBridge(_) + | QueryExpr::QueryTimestamp => {} QueryExpr::Column(_) | QueryExpr::Literal(_) | QueryExpr::Compare { .. } @@ -262,8 +264,8 @@ fn all_targets_missing_core_lowers() { #[test] fn scalar_threshold_comparisons_lower_to_binaryop_scalar() { // ~822/949 corpus queries are ` `. The numeric - // threshold is now a `PromqlScalar` operand of the `BinaryOp` (issue #35) — the - // single biggest unblock for real alerts. + // threshold is now a `PromqlScalarBridge` 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", @@ -273,7 +275,7 @@ fn scalar_threshold_comparisons_lower_to_binaryop_scalar() { panic!("expected a BinaryOp for {q:?}"); }; assert!( - matches!(rhs.as_ref(), QueryExpr::PromqlScalar(_)), + matches!(rhs.as_ref(), QueryExpr::PromqlScalarBridge(_)), "scalar threshold operand for {q:?}, got {rhs:?}" ); } @@ -328,7 +330,7 @@ fn vector_literal_lowers_to_a_labelless_vector() { let QueryExpr::PromqlVectorFromScalar(inner) = &qe else { panic!("expected PromqlVectorFromScalar, got {qe:?}"); }; - assert!(matches!(inner.as_ref(), QueryExpr::PromqlScalar(v) if *v == 1.0)); + assert_eq!(inner.as_promql_scalar(), Some(1.0)); // The result is a vector: it carries a time index (unlike a bare scalar). assert!(qe.output_schema().unwrap().time_index.is_some()); } diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index b0886c36..d8673f77 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -98,7 +98,7 @@ fn collect(e: &QueryExpr, out: &mut Vec) { } // `AggIntent` only ever lives in `Aggregate.measures`, never in a // scalar position (issue #205) — nothing to collect there. - QueryExpr::Scan { .. } | QueryExpr::PromqlScalar(_) | QueryExpr::QueryTimestamp => {} + QueryExpr::Scan { .. } | QueryExpr::PromqlScalarBridge(_) | QueryExpr::QueryTimestamp => {} QueryExpr::Column(_) | QueryExpr::Literal(_) | QueryExpr::Compare { .. } @@ -143,11 +143,13 @@ fn has bool>(e: &QueryExpr, pred: F) -> bool { intents(e).iter().any(pred) } -/// Whether the tree contains a `Mul`-by-`PromqlScalar(-1)` anywhere — the shape unary +/// Whether the tree contains a `Mul`-by-`PromqlScalarBridge(-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::PromqlScalar(v) if (*v + 1.0).abs() < 1e-12); + let is_neg_one = |q: &QueryExpr| { + q.as_promql_scalar() + .is_some_and(|v| (v + 1.0).abs() < 1e-12) + }; match e { QueryExpr::BinaryOp { op, lhs, rhs, .. } => { (*op == BinaryOpKind::Arithmetic(ArithmeticOpKind::Mul) @@ -614,7 +616,7 @@ fn vector_comparison_filters() { 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 `PromqlScalar(-1)`. These are + // BinaryOp of the (label-preserving) vector against `PromqlScalarBridge(-1)`. These are // the five cases the old `__GAP` test pinned as rejected. for q in [ "-rate(http_errors_total[5m])", @@ -624,14 +626,14 @@ fn unary_negation_lowers_as_multiply_by_minus_one() { "sum(-node_cpu_seconds_total)", ] { let qe = ok(q); - // A `Mul`-by-`-1` against a `PromqlScalar(-1)` appears somewhere in every tree. + // A `Mul`-by-`-1` against a `PromqlScalarBridge(-1)` appears somewhere in every tree. assert!( negates_via_scalar(&qe), "no `* -1` negation found in {q}: {qe:?}" ); } - // `-some_metric` at the root: `Scan * PromqlScalar(-1)`, schema follows the vector. + // `-some_metric` at the root: `Scan * PromqlScalarBridge(-1)`, schema follows the vector. let QueryExpr::BinaryOp { op, lhs, @@ -647,8 +649,9 @@ fn unary_negation_lowers_as_multiply_by_minus_one() { "vector on the left" ); assert!( - matches!(rhs.as_ref(), QueryExpr::PromqlScalar(v) if (*v + 1.0).abs() < 1e-12), - "negation multiplies by PromqlScalar(-1), got {rhs:?}" + rhs.as_promql_scalar() + .is_some_and(|v| (v + 1.0).abs() < 1e-12), + "negation multiplies by PromqlScalarBridge(-1), got {rhs:?}" ); assert!( vector_match.is_none(), @@ -686,11 +689,10 @@ fn unary_negation_lowers_as_multiply_by_minus_one() { #[test] fn unary_negation_of_constant_folds_to_scalar() { // `-(10*1024*1024)` — the operand is constant-foldable, so negation collapses - // to a single negated `PromqlScalar` leaf (no `BinaryOp`), just like a bare literal. - assert!(matches!( - ok("-(10*1024*1024)"), - QueryExpr::PromqlScalar(v) if (v + 10_485_760.0).abs() < 1e-6 - )); + // to a single negated `PromqlScalarBridge` leaf (no `BinaryOp`), just like a bare literal. + assert!(ok("-(10*1024*1024)") + .as_promql_scalar() + .is_some_and(|v| (v + 10_485_760.0).abs() < 1e-6)); } #[test] @@ -749,9 +751,9 @@ fn count_maps_to_cardinality_and_inherits_accuracy() { #[test] fn scalar_literal_operand_lowers_as_binaryop_scalar() { - // Issue #35: ` op ` — the numeric threshold is a `PromqlScalar` - // operand of the `BinaryOp`, and constant arithmetic (`10*1024*1024`) is - // folded. The output schema is the vector side's. + // Issue #35: ` op ` — the numeric threshold is a + // `PromqlScalarBridge` 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:?}"); @@ -762,7 +764,8 @@ fn scalar_literal_operand_lowers_as_binaryop_scalar() { "vector on the left" ); assert!( - matches!(rhs.as_ref(), QueryExpr::PromqlScalar(v) if (*v - 10_485_760.0).abs() < 1e-6), + rhs.as_promql_scalar() + .is_some_and(|v| (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). @@ -772,13 +775,15 @@ fn scalar_literal_operand_lowers_as_binaryop_scalar() { #[test] fn scalar_arithmetic_scales_the_vector() { // `rate(m[5m]) * 100` — a unit conversion. Arithmetic BinaryOp of the vector - // with a `PromqlScalar(100)`. + // with a `PromqlScalarBridge(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::Arithmetic(ArithmeticOpKind::Mul)); - assert!(matches!(rhs.as_ref(), QueryExpr::PromqlScalar(v) if (*v - 100.0).abs() < 1e-9)); + assert!(rhs + .as_promql_scalar() + .is_some_and(|v| (v - 100.0).abs() < 1e-9)); } // ───────────────────────────────────────────────────────────────────────────── @@ -1695,10 +1700,10 @@ fn clamp_and_round_carry_their_params() { #[test] fn pi_lowers_to_a_scalar_constant() { - // `pi()` is the constant π — a `PromqlScalar` leaf, not a `Math` intent. - assert!( - matches!(ok("pi()"), QueryExpr::PromqlScalar(v) if (v - std::f64::consts::PI).abs() < 1e-12) - ); + // `pi()` is the constant π — a `PromqlScalarBridge` leaf, not a `Math` intent. + assert!(ok("pi()") + .as_promql_scalar() + .is_some_and(|v| (v - std::f64::consts::PI).abs() < 1e-12)); } // ───────────────────────────────────────────────────────────────────────────── @@ -1826,7 +1831,7 @@ fn vector_promotes_a_scalar_to_a_vector() { let QueryExpr::PromqlVectorFromScalar(inner) = &qe else { panic!("expected PromqlVectorFromScalar, got {qe:?}"); }; - assert!(matches!(inner.as_ref(), QueryExpr::PromqlScalar(v) if *v == 1.0)); + assert_eq!(inner.as_promql_scalar(), Some(1.0)); // Vector-typed: schema has a time index (a scalar leaf has none). let sch = qe.output_schema().unwrap(); assert!(sch.time_index.is_some()); @@ -1842,7 +1847,7 @@ fn scalar_collapses_a_vector_to_a_scalar() { }; let (metric, _) = first_scan(inner); assert_eq!(metric, "node_load1"); - // PromqlScalar-typed: single `value` column, no time index. + // PromqlScalarBridge-typed: single `value` column, no time index. let sch = qe.output_schema().unwrap(); assert!(sch.time_index.is_none()); assert_eq!(sch.columns.len(), 1); @@ -2234,25 +2239,28 @@ fn sort_by_label_desc_is_descending() { #[test] fn min_of_max_of_fold_constant_scalars() { // `min_of`/`max_of` are n-ary scalar reducers. When every argument is a - // constant they constant-fold to a `PromqlScalar` leaf, just like scalar + // constant they constant-fold to a `PromqlScalarBridge` leaf, just like scalar // arithmetic (#35) — the only form the intent algebra can hold (#89). - assert!(matches!(ok("min_of(3, 5)"), QueryExpr::PromqlScalar(v) if v == 3.0)); - assert!(matches!(ok("max_of(3, 5)"), QueryExpr::PromqlScalar(v) if v == 5.0)); - assert!(matches!(ok("min_of(-2, -5)"), QueryExpr::PromqlScalar(v) if v == -5.0)); + assert_eq!(ok("min_of(3, 5)").as_promql_scalar(), Some(3.0)); + assert_eq!(ok("max_of(3, 5)").as_promql_scalar(), Some(5.0)); + assert_eq!(ok("min_of(-2, -5)").as_promql_scalar(), Some(-5.0)); // Nested folds and use as a threshold operand. - assert!(matches!(ok("max_of(min_of(2, 3), 10)"), QueryExpr::PromqlScalar(v) if v == 10.0)); + assert_eq!( + ok("max_of(min_of(2, 3), 10)").as_promql_scalar(), + Some(10.0) + ); let qe = ok("up > max_of(1, 2)"); let QueryExpr::BinaryOp { rhs, .. } = &qe else { panic!("{qe:?}") }; - assert!(matches!(rhs.as_ref(), QueryExpr::PromqlScalar(v) if *v == 2.0)); + assert_eq!(rhs.as_promql_scalar(), Some(2.0)); } #[test] fn min_of_max_of_ignore_nan_like_the_min_max_aggregators() { // A NaN argument is skipped (Prometheus `min`/`max` NaN semantics). - assert!(matches!(ok("max_of(3, NaN)"), QueryExpr::PromqlScalar(v) if v == 3.0)); - assert!(matches!(ok("min_of(NaN, 3)"), QueryExpr::PromqlScalar(v) if v == 3.0)); + assert_eq!(ok("max_of(3, NaN)").as_promql_scalar(), Some(3.0)); + assert_eq!(ok("min_of(NaN, 3)").as_promql_scalar(), Some(3.0)); } #[test] diff --git a/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs b/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs index bf8651a5..eccc01f3 100644 --- a/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs +++ b/crates/frontend-sql/tests/data_quality_check/synthetic_packet_trace.rs @@ -109,7 +109,9 @@ fn intents(e: &QueryExpr) -> Vec { QueryExpr::PromqlVectorFromScalar(inner) | QueryExpr::PromqlScalarFromVector(inner) => { go(inner, out) } - QueryExpr::Scan { .. } | QueryExpr::PromqlScalar(_) | QueryExpr::QueryTimestamp => {} + QueryExpr::Scan { .. } + | QueryExpr::PromqlScalarBridge(_) + | QueryExpr::QueryTimestamp => {} // Scalar expression variants (issue #205): `AggIntent` only ever // lives in `Aggregate.measures`, never nested inside a scalar // expression tree, so there's nothing to recurse into here. diff --git a/crates/frontend-sql/tests/netflow/netflow.rs b/crates/frontend-sql/tests/netflow/netflow.rs index ea85bc77..8763cb7b 100644 --- a/crates/frontend-sql/tests/netflow/netflow.rs +++ b/crates/frontend-sql/tests/netflow/netflow.rs @@ -298,7 +298,7 @@ fn visit(qe: &QueryExpr, f: &mut impl FnMut(&QueryExpr)) { QueryExpr::PromqlVectorFromScalar(child) | QueryExpr::PromqlScalarFromVector(child) => { visit(child, f) } - QueryExpr::Scan { .. } | QueryExpr::PromqlScalar(_) | QueryExpr::QueryTimestamp => {} + QueryExpr::Scan { .. } | QueryExpr::PromqlScalarBridge(_) | QueryExpr::QueryTimestamp => {} // Scalar expression variants (issue #205) aren't relational nodes; // this visitor only walks the relational tree, so stop here. QueryExpr::Column(_) diff --git a/crates/integration-tests/tests/binary_op.rs b/crates/integration-tests/tests/binary_op.rs index 217e8022..406023de 100644 --- a/crates/integration-tests/tests/binary_op.rs +++ b/crates/integration-tests/tests/binary_op.rs @@ -229,13 +229,13 @@ fn q21_div_two_sum_by_job() { } // #36 — unary negation lowers as `expr * -1`: a Mul BinaryOp of the vector -// against PromqlScalar(-1), no vector match. The vector side keeps its schema. +// against PromqlScalarBridge(-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::Arithmetic(ArithmeticOpKind::Mul), lhs: Rc::new(scan("some_metric", &[])), - rhs: Rc::new(QueryExpr::PromqlScalar(-1.0)), + rhs: Rc::new(QueryExpr::promql_scalar(-1.0)), vector_match: None, }; assert_eq!(lower("-some_metric"), expected); @@ -253,7 +253,7 @@ fn q36_sum_of_negation_nests() { child: Rc::new(QueryExpr::BinaryOp { op: BinaryOpKind::Arithmetic(ArithmeticOpKind::Mul), lhs: Rc::new(scan("node_cpu_seconds_total", &[])), - rhs: Rc::new(QueryExpr::PromqlScalar(-1.0)), + rhs: Rc::new(QueryExpr::promql_scalar(-1.0)), vector_match: None, }), }; diff --git a/crates/types/src/dag_export.rs b/crates/types/src/dag_export.rs index f72483b5..2f53fb7b 100644 --- a/crates/types/src/dag_export.rs +++ b/crates/types/src/dag_export.rs @@ -144,12 +144,17 @@ fn build(expr: &QueryExpr, nodes: &mut Vec) -> u32 { }); push_node(nodes, "Scan", label, detail, vec![]) } - QueryExpr::PromqlScalar(v) => { - let detail = serde_json::json!({ "value": v }); + // The bridged child is a scalar-sub-language node (issue #220), not + // an operator node `build` can recurse into — serialize it as opaque + // `detail` JSON, same as every other scalar-typed field + // (`Filter.pred`, `Project.cols`, …) rather than pushing it as a + // separate DAG node. + QueryExpr::PromqlScalarBridge(inner) => { + let detail = serde_json::json!({ "value": inner }); push_node( nodes, - "PromqlScalar", - format!("PromqlScalar({v})"), + "PromqlScalarBridge", + format!("PromqlScalarBridge({inner:?})"), detail, vec![], ) diff --git a/crates/types/src/pre_asap/binder.rs b/crates/types/src/pre_asap/binder.rs index a473fedb..75d1dfa1 100644 --- a/crates/types/src/pre_asap/binder.rs +++ b/crates/types/src/pre_asap/binder.rs @@ -143,7 +143,10 @@ fn leftmost_scan_name(tree: &UnresolvedQueryExpr) -> Option<&str> { super::query_expr::Source::TimeSeries { metric } => metric.as_str(), super::query_expr::Source::Table { table_ref } => table_ref.as_str(), }), - QE::PromqlScalar(_) | QE::QueryTimestamp => None, + // A scalar bridge's child is a scalar-sub-language leaf (in practice + // always a `Literal`, issue #220) — never a `Scan`, same as + // `QueryTimestamp`. + QE::PromqlScalarBridge(_) | QE::QueryTimestamp => None, QE::PromqlVectorFromScalar(child) | QE::PromqlScalarFromVector(child) => { leftmost_scan_name(child) } @@ -292,7 +295,13 @@ pub(crate) fn collect_referenced_columns(tree: &UnresolvedQueryExpr) -> Vec {} + QE::QueryTimestamp => {} + // The bridged child is a genuine scalar-sub-language position now + // (issue #220) — peel its column refs off with `named`, same as + // every other scalar-typed field (`Scan.predicates`, + // `Filter.pred`, …). In practice it's always a `Literal`, which + // references no columns, so this is a no-op today. + QE::PromqlScalarBridge(inner) => named(inner, out), QE::PromqlVectorFromScalar(child) | QE::PromqlScalarFromVector(child) => { walk(child, out) } diff --git a/crates/types/src/pre_asap/canonicalize.rs b/crates/types/src/pre_asap/canonicalize.rs index 7487ceb4..7432038b 100644 --- a/crates/types/src/pre_asap/canonicalize.rs +++ b/crates/types/src/pre_asap/canonicalize.rs @@ -82,7 +82,10 @@ fn rc_mut(r: &mut Rc) -> &mut QueryExpr { fn children_mut(expr: &mut QueryExpr) -> Vec<&mut QueryExpr> { use QueryExpr::*; match expr { - Scan { .. } | PromqlScalar(_) | QueryTimestamp => vec![], + // `PromqlScalarBridge`'s child is a scalar-sub-language node (issue + // #220), not the relational skeleton — same "no children to recurse + // into" treatment as the scalar variants below. + Scan { .. } | QueryTimestamp | PromqlScalarBridge(_) => vec![], PromqlVectorFromScalar(c) | PromqlScalarFromVector(c) => vec![rc_mut(c)], PromqlRelabel { child, .. } | Filter { child, .. } diff --git a/crates/types/src/pre_asap/cse.rs b/crates/types/src/pre_asap/cse.rs index 2939ae3b..13100850 100644 --- a/crates/types/src/pre_asap/cse.rs +++ b/crates/types/src/pre_asap/cse.rs @@ -140,7 +140,7 @@ impl InternTable { /// Coarse structural hash used only to bucket [`InternTable::intern`]'s /// candidate search — never the actual sharing decision (`PartialEq` is). /// -/// `QueryExpr` carries `f64`s (`PromqlScalar`, `AggIntent::Quantile.q`, …), so it +/// `QueryExpr` carries `f64`s (`Literal(ScalarValue::Float64)`, `AggIntent::Quantile.q`, …), so it /// cannot derive `std::hash::Hash`. Serializing to a canonical JSON string /// and hashing that sidesteps the `f64` problem the same way /// `dag_export.rs`'s own `structural_hash` does — a deliberately independent @@ -199,7 +199,7 @@ fn intern_bottom_up(table: &mut InternTable, expr: QueryExpr) -> Rc { fn rebuild_children(table: &mut InternTable, expr: QueryExpr) -> QueryExpr { use QueryExpr::*; match expr { - Scan { .. } | PromqlScalar(_) | QueryTimestamp => expr, + Scan { .. } | QueryTimestamp => expr, PromqlVectorFromScalar(c) => PromqlVectorFromScalar(intern_child(table, c)), PromqlScalarFromVector(c) => PromqlScalarFromVector(intern_child(table, c)), PromqlRelabel { dst, value, child } => PromqlRelabel { @@ -331,6 +331,11 @@ fn rebuild_children(table: &mut InternTable, expr: QueryExpr) -> QueryExpr { rhs: intern_child(table, rhs), vector_match, }, + // `PromqlScalarBridge`'s child is a scalar-sub-language node (issue + // #220) — same "never descended into" treatment as the scalar + // variants below; the whole bridge node is still interned as a unit + // by the `table.intern(rebuilt)` call in `intern_bottom_up`. + PromqlScalarBridge(_) => expr, // Scalar variants (issue #205) — never descended into; see the // module doc's "Algorithm" section on scope. Left byte-for-byte // unchanged: predicate / project-list / sort-key / window-arg diff --git a/crates/types/src/pre_asap/query_expr.rs b/crates/types/src/pre_asap/query_expr.rs index e6ee65ae..6d01ede2 100644 --- a/crates/types/src/pre_asap/query_expr.rs +++ b/crates/types/src/pre_asap/query_expr.rs @@ -522,10 +522,25 @@ pub enum QueryExpr { predicates: Vec>, schema: C::ScanSchema, }, - /// 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). - PromqlScalar(f64), + /// A scalar sub-expression sitting in an **operator-tree position** — a + /// [`BinaryOp`](Self::BinaryOp) operand for ` op ` + /// thresholds / unit conversions (#35), a + /// [`PromqlVectorFromScalar`](Self::PromqlVectorFromScalar) child, or a + /// whole query's root (a bare PromQL scalar query, e.g. `5`). + /// + /// Formerly its own leaf variant, `PromqlScalar(f64)`. Issue #220: that + /// variant held exactly the same value [`Literal`](Self::Literal) does + /// (every PromQL scalar is `f64`), duplicating it for no reason but + /// *which tree position* it was allowed to appear in. This wrapper + /// carries that position instead of the value — the inner node is an + /// ordinary scalar sub-language expression (in practice always + /// `Literal(ScalarValue::Float64(_))`, since a front end only ever + /// constructs this fully constant-folded — see + /// [`promql_scalar`](Self::promql_scalar)) — and is what `output_schema`, + /// `canonicalize`, and `resolve` now key off to tell "this operand has + /// its own row schema" from "this is a nested scalar leaf with none," + /// in place of the old `PromqlScalar` vs. `Literal` variant tag. + PromqlScalarBridge(Rc>), /// The query **evaluation time** as a scalar (PromQL `time()`) — a runtime /// value, not a constant. Also the implicit input of the no-argument @@ -812,6 +827,30 @@ pub enum QueryExpr { } impl QueryExpr { + /// Construct the [`PromqlScalarBridge`](Self::PromqlScalarBridge) leaf + /// for a bare PromQL numeric literal / folded constant scalar (issue + /// #220) — `Literal(ScalarValue::Float64(v))` at an operator-tree + /// position. The one constructor every front end / test that used to + /// write `QueryExpr::PromqlScalar(v)` should use instead. + pub fn promql_scalar(v: f64) -> Self { + QueryExpr::PromqlScalarBridge(Rc::new(QueryExpr::Literal(ScalarValue::Float64(v)))) + } + + /// The value of a [`PromqlScalarBridge`](Self::PromqlScalarBridge) leaf + /// wrapping a plain `Literal(ScalarValue::Float64(_))` — every one a + /// front end constructs today (see [`promql_scalar`](Self::promql_scalar)). + /// `None` for any other shape, including a `PromqlScalarBridge` wrapping + /// something else (not constructed today, but not precluded by the type). + pub fn as_promql_scalar(&self) -> Option { + match self { + QueryExpr::PromqlScalarBridge(inner) => match inner.as_ref() { + QueryExpr::Literal(ScalarValue::Float64(v)) => Some(*v), + _ => None, + }, + _ => None, + } + } + /// If this expression is a `BoolAnd`, return its elements; otherwise a /// single-element slice containing `self`. pub fn conjuncts(&self) -> &[QueryExpr] { @@ -1102,11 +1141,14 @@ impl QueryExpr { Ok(out) } - // A scalar constant has no series — model it as a single `value` - // column so it can sit as a `BinaryOp` operand. - // Both scalar leaves — a constant and the eval time — are a single - // `value` column with no labels. - QueryExpr::PromqlScalar(_) | QueryExpr::QueryTimestamp => Ok(Schema { + // A scalar bridge has no series — model it as a single `value` + // column so it can sit as a `BinaryOp` operand. Both scalar + // leaves — a bridged scalar sub-expression and the eval time — + // are a single `value` column with no labels. Every + // `PromqlScalarBridge` constructed today wraps a plain + // `Literal(Float64)` (issue #220), so the schema doesn't need to + // inspect the inner node. + QueryExpr::PromqlScalarBridge(_) | QueryExpr::QueryTimestamp => Ok(Schema { columns: vec![Column::new("value", DataType::Float64, false)], time_index: None, unique_keys: Vec::new(), @@ -1141,7 +1183,9 @@ impl QueryExpr { // non-scalar side. QueryExpr::BinaryOp { lhs, rhs, .. } => match (lhs.as_ref(), rhs.as_ref()) { ( - QueryExpr::PromqlScalar(_) | QueryExpr::QueryTimestamp | QueryExpr::PromqlScalarFromVector(_), + QueryExpr::PromqlScalarBridge(_) + | QueryExpr::QueryTimestamp + | QueryExpr::PromqlScalarFromVector(_), r, ) => r.output_schema(), (l, _) => l.output_schema(), @@ -1910,4 +1954,96 @@ mod tests { "UNION does not preserve row identity" ); } + + // ── PromqlScalarBridge / Literal dedup (issue #220) ───────────────────── + + /// `QueryExpr::promql_scalar(v)` — what every front end now constructs in + /// place of the old `PromqlScalar(v)` leaf — wraps exactly + /// `Literal(ScalarValue::Float64(v))`: the same value a SQL-emitted typed + /// float literal in a scalar-sub-language position would carry, just at a + /// different tree position. `as_promql_scalar` is the round-trip inverse. + #[test] + fn promql_scalar_bridges_a_literal_float_at_an_operator_position() { + let bridge = QueryExpr::::promql_scalar(2.5); + assert_eq!( + bridge, + QueryExpr::PromqlScalarBridge(Rc::new(QueryExpr::Literal(ScalarValue::Float64(2.5)))) + ); + assert_eq!(bridge.as_promql_scalar(), Some(2.5)); + + // The same value a SQL `Compare`/`Arithmetic` operand would carry, in + // its native (unwrapped, no row schema) scalar-sub-language position — + // no longer a different variant, just not bridged to this tree + // position. + let sql_literal = QueryExpr::::Literal(ScalarValue::Float64(2.5)); + assert_eq!(bridge.as_promql_scalar(), Some(2.5)); + assert_ne!( + bridge, sql_literal, + "bridge and bare literal are distinct nodes" + ); + // Not every shape is a scalar bridge: neither a bare `Literal` nor an + // operator node reports a value. + assert_eq!(sql_literal.as_promql_scalar(), None); + assert_eq!(scan(vec![], None, vec![]).as_promql_scalar(), None); + } + + /// Pins the tree-position distinction issue #220 asks for: the very same + /// `Literal(ScalarValue::Float64(_))` value has a row schema when it sits + /// at the operator-tree position (wrapped in `PromqlScalarBridge` — a + /// `BinaryOp` operand, `PromqlVectorFromScalar` child, or a query root), + /// and has none when it sits bare, in a scalar-sub-language position + /// (`Compare`/`Arithmetic`/… operand) — no longer decided by which of two + /// duplicate variants was used, only by whether the wrapper is present. + #[test] + fn row_schema_rides_on_the_bridge_wrapper_not_the_literal_variant() { + let bridged = QueryExpr::::promql_scalar(42.0); + let schema = bridged.output_schema().expect("bridge has a row schema"); + assert_eq!(schema.columns.len(), 1); + assert_eq!(schema.columns[0].name, "value"); + assert_eq!(schema.columns[0].dtype, DataType::Float64); + assert!(schema.time_index.is_none()); + + // The identical value, unwrapped (the scalar-sub-language position a + // `Compare`/`Arithmetic` operand would occupy) has no row schema of + // its own — it's a construction bug to call `output_schema` on it + // directly, caught as `ScalarHasNoRowSchema` rather than panicking. + let bare = QueryExpr::::Literal(ScalarValue::Float64(42.0)); + assert!(matches!( + bare.output_schema(), + Err(QueryExprError::ScalarHasNoRowSchema) + )); + } + + /// `BinaryOp`'s schema derivation follows the non-scalar (vector) side + /// when the other operand is a `PromqlScalarBridge`, and a `VectorMatch` + /// modifier survives unchanged alongside it — the relational binary-op + /// path (issue #220's Instance 2, left as follow-up) is untouched by the + /// Instance-1 `PromqlScalar` → `PromqlScalarBridge` collapse. + #[test] + fn binary_op_schema_follows_the_vector_side_over_a_scalar_bridge_with_vector_match_intact() { + let vector = scan( + vec![ + col("host", DataType::Utf8, false), + col("value", DataType::Float64, false), + ], + None, + vec![], + ); + let vm = VectorMatch { + kind: VectorMatchKind::On, + labels: vec!["host".into()], + grouping: None, + }; + let op = QueryExpr::BinaryOp { + op: BinaryOpKind::Compare(CompareOpKind::Gt), + lhs: Rc::new(vector.clone()), + rhs: Rc::new(QueryExpr::promql_scalar(1.0)), + vector_match: Some(vm.clone()), + }; + assert_eq!(op.output_schema().unwrap(), vector.output_schema().unwrap()); + let QueryExpr::BinaryOp { vector_match, .. } = &op else { + unreachable!() + }; + assert_eq!(vector_match.as_ref(), Some(&vm)); + } } diff --git a/crates/types/src/pre_asap/resolve.rs b/crates/types/src/pre_asap/resolve.rs index 0251399d..bfdc8836 100644 --- a/crates/types/src/pre_asap/resolve.rs +++ b/crates/types/src/pre_asap/resolve.rs @@ -122,7 +122,15 @@ fn resolve( } } - QE::PromqlScalar(v) => QE::PromqlScalar(*v), + // `PromqlScalarBridge`'s child is a scalar-sub-language node (issue + // #220) sitting at this operator-tree position — resolved through + // `resolve_expr`, same as every other scalar position (`Predicate`, + // `ProjectItem.expr`, …), not the operator walk. In practice it's + // always a `Literal`, which has no `ColumnRef` to resolve, so + // `fallback` is never actually consulted here. + QE::PromqlScalarBridge(inner) => { + QE::PromqlScalarBridge(Rc::new(resolve_expr(inner, fallback)?)) + } QE::QueryTimestamp => QE::QueryTimestamp, QE::PromqlVectorFromScalar(child) => { @@ -552,3 +560,62 @@ fn resolve_agg_intent( }, }) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::pre_asap::expr_ir::CompareOpKind; + use crate::pre_asap::query_expr::{ + BinaryOpKind, QueryExpr, Source, VectorMatch, VectorMatchKind, + }; + + /// `resolve_root` over a `BinaryOp { , PromqlScalarBridge, vector_match }` + /// (issue #220): the bridged scalar operand resolves through the same + /// generic walk as every other node (its `Literal` child has no + /// `ColumnRef` to resolve, so it comes through unchanged), the vector + /// side's `ColumnRef`s resolve positionally, and the `VectorMatch` + /// modifier on the relational binary-op path survives resolution + /// untouched — Instance 2 of #220 (`BinaryOp` vs `Compare`/`Arithmetic`) + /// is out of scope for this change, so this pins that its behavior is + /// unaffected by the Instance-1 collapse. + #[test] + fn resolve_root_threads_a_scalar_bridge_operand_and_preserves_vector_match() { + let vm = VectorMatch { + kind: VectorMatchKind::Ignoring, + labels: vec!["job".into()], + grouping: None, + }; + let unresolved: UnresolvedQueryExpr = QueryExpr::BinaryOp { + op: BinaryOpKind::Compare(CompareOpKind::Gt), + lhs: Rc::new(UnresolvedQueryExpr::Scan { + source: Source::TimeSeries { + metric: "up".into(), + }, + predicates: vec![], + schema: None, + }), + rhs: Rc::new(UnresolvedQueryExpr::promql_scalar(1.0)), + vector_match: Some(vm.clone()), + }; + + let resolved = resolve_root(&unresolved).expect("resolves"); + let QueryExpr::BinaryOp { + lhs, + rhs, + vector_match, + .. + } = &resolved + else { + panic!("expected a resolved BinaryOp, got {resolved:?}"); + }; + assert!(matches!(lhs.as_ref(), QueryExpr::Scan { .. })); + assert_eq!(rhs.as_promql_scalar(), Some(1.0)); + assert_eq!(vector_match.as_ref(), Some(&vm)); + + // Schema derivation still follows the vector side post-resolution. + assert_eq!( + resolved.output_schema().unwrap(), + lhs.output_schema().unwrap() + ); + } +} diff --git a/docs/pre-asap-ir.md b/docs/pre-asap-ir.md index 8b6afd96..d6ece485 100644 --- a/docs/pre-asap-ir.md +++ b/docs/pre-asap-ir.md @@ -39,7 +39,7 @@ to one source language. - [`Concat`](#concat) — exact, untyped `UNION ALL` of union-compatible branches. **[PromQL-specific nodes](#promql-specific-nodes)** -- [`PromqlScalar`](#promqlscalar) — a scalar constant leaf. +- [`PromqlScalarBridge`](#promqlscalarbridge) — a scalar sub-expression at an operator-tree position. - [`QueryTimestamp`](#querytimestamp) — the query evaluation time as a scalar (PromQL `time()`). - [`PromqlVectorFromScalar`](#promqlvectorfromscalar) — promotes a scalar to a label-less instant vector. - [`PromqlScalarFromVector`](#promqlscalarfromvector) — collapses a single-series vector to a scalar. @@ -405,16 +405,20 @@ histogram_quantiles(rate(http_request_duration_seconds_bucket[5m]), "le", 0.5, 0 ## PromQL-specific nodes -### PromqlScalar +### PromqlScalarBridge -A scalar constant leaf — a PromQL number literal, or a folded constant scalar expression. -Appears as a `BinaryOp` operand for ` op ` thresholds and unit conversions. +A scalar sub-expression (issue #220: in practice always `Literal(ScalarValue::Float64(_))` — +a PromQL number literal, or a folded constant scalar expression) sitting at an **operator-tree +position** — a `BinaryOp` operand for ` op ` thresholds and unit conversions, +a `PromqlVectorFromScalar` child, or a whole query's root. This wrapper is what marks the +position; it no longer duplicates `Literal`'s value the way the old `PromqlScalar(f64)` variant +did. ```promql up > 1 ``` -**Fields:** a single unnamed `f64` — the constant value. +**Fields:** a single unnamed child `QueryExpr` — the wrapped scalar sub-expression. ### QueryTimestamp