From 0399a497379eeb0561515708ee6959d5da47366a Mon Sep 17 00:00:00 2001 From: zz_y Date: Sun, 5 Jul 2026 10:08:35 -0600 Subject: [PATCH] feat(promql): constant-fold min_of / max_of scalar reducers (#89) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `min_of`/`max_of` are n-ary *scalar* reducers (Scalar…→Scalar) that parsed but were rejected. When I split #89 off #51 I expected they'd need a scalar min/max IR node and wouldn't help the corpus — but the corpus splits in two: 14 of the 27 uses are bare constant forms (`min_of(3, 5)`, `max_of(-2, -5)`, NaN cases) in ordinary scalar positions, and only the other 13 are nested with `step()`/`range()` inside dynamic range / offset positions that are themselves unsupported (unrelated to min_of/max_of). So fold the constant case into the existing `Scalar` leaf, the same mechanism as scalar arithmetic (#35): - `num_expr` gains a `min_of`/`max_of` arm that folds when every argument is a constant scalar, reducing with `f64::min`/`max` (which ignore NaN, matching PromQL's `min`/`max` semantics). A non-constant argument (`step()`) fails the recursive fold and propagates the error, so those stay rejected — there is no scalar min/max node and they only occur in unsupported dynamic contexts. - `walk` folds a bare top-level `min_of(consts…)` query to a `Scalar` leaf; operand/nested positions already route through `num_expr` via `scalar_or_vector`. Tests: conformance §U — constant folds (incl. negatives, nesting, threshold operand), NaN-ignoring, and a `__GAP` pinning that non-constant (`step()`) forms stay rejected. Flipped the old blanket-rejection pin. Co-Authored-By: Claude Opus 4.8 --- crates/frontend-promql/src/promql.rs | 32 ++++++++++++++++ .../tests/promql_conformance.rs | 38 +++++++++++++++---- 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/crates/frontend-promql/src/promql.rs b/crates/frontend-promql/src/promql.rs index 7131ae21..d8e9cb1e 100644 --- a/crates/frontend-promql/src/promql.rs +++ b/crates/frontend-promql/src/promql.rs @@ -197,6 +197,11 @@ 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 `Scalar` + // leaf; a non-constant argument makes `num_expr` fail → rejected (#89). + Expr::Call(call) if is_scalar_reducer_fn(call.func.name) => { + Ok(L2::Scalar(num_expr(expr)?)) + } Expr::Call(call) => walk_call(call), Expr::Binary(bin) => walk_binary(bin), Expr::Paren(p) => walk(&p.expr), @@ -1433,6 +1438,27 @@ fn num_expr(expr: &Expr) -> Result { )) } } + // `min_of`/`max_of` are n-ary *scalar* reducers (issue #89). Fold them + // when every argument is itself a constant scalar — this is the only + // form the intent algebra can hold (there is no scalar min/max node). A + // non-constant argument (`min_of(step(), 1s)`) fails the recursive fold + // and propagates the error, so it stays rejected. `f64::min`/`max` + // ignore NaN, matching PromQL's `min`/`max` NaN semantics. + Expr::Call(c) if is_scalar_reducer_fn(c.func.name) => { + let reduce = if c.func.name == "min_of" { + f64::min + } else { + f64::max + }; + c.args + .args + .iter() + .map(|a| num_expr(a)) + .reduce(|acc, v| Ok(reduce(acc?, v?))) + .ok_or_else(|| { + LoweringError::MissingArgument(format!("{} needs an argument", c.func.name)) + })? + } other => Err(LoweringError::InvalidParameter(format!( "expected a numeric scalar, got {:?}", std::mem::discriminant(other) @@ -1440,6 +1466,12 @@ fn num_expr(expr: &Expr) -> Result { } } +/// The n-ary scalar min/max reducers, foldable when all arguments are constant +/// scalars (issue #89). +fn is_scalar_reducer_fn(name: &str) -> bool { + matches!(name, "min_of" | "max_of") +} + /// 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 { diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index 4d9cfbc4..f09e987d 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -1573,11 +1573,35 @@ fn sort_by_label_desc_is_descending() { } #[test] -fn min_of_max_of_are_rejected__GAP() { - // `min_of`/`max_of` are n-ary *scalar* reducers, almost always nested with - // the `step()`/`range()` scalar helpers inside range/offset positions that - // are themselves unsupported. They need a scalar-reduction node — deferred - // to #89, not mislowered. - let _ = rejected("min_of(1, 2, 3)"); - let _ = rejected("max_of(1, 2)"); +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 `Scalar` leaf, just like scalar + // arithmetic (#35) — the only form the intent algebra can hold (#89). + assert!(matches!(ok("min_of(3, 5)"), QueryExpr::Scalar(v) if v == 3.0)); + assert!(matches!(ok("max_of(3, 5)"), QueryExpr::Scalar(v) if v == 5.0)); + assert!(matches!(ok("min_of(-2, -5)"), QueryExpr::Scalar(v) if v == -5.0)); + // Nested folds and use as a threshold operand. + assert!(matches!(ok("max_of(min_of(2, 3), 10)"), QueryExpr::Scalar(v) if v == 10.0)); + let qe = ok("up > max_of(1, 2)"); + let QueryExpr::BinaryOp { rhs, .. } = &qe else { + panic!("{qe:?}") + }; + assert!(matches!(rhs.as_ref(), QueryExpr::Scalar(v) if *v == 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::Scalar(v) if v == 3.0)); + assert!(matches!(ok("min_of(NaN, 3)"), QueryExpr::Scalar(v) if v == 3.0)); +} + +#[test] +fn non_constant_min_of_max_of_is_rejected__GAP() { + // A dynamic argument (`step()` — itself unsupported, #89) can't be folded to + // a constant and there is no scalar min/max node, so it stays rejected + // rather than mislowered. These forms also only appear inside unsupported + // dynamic range / offset positions in the corpus. + let _ = rejected("min_of(step(), 1s)"); + let _ = rejected("max_of(min_of(step() + 1, 1h), 1ms)"); }