Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions crates/frontend-promql/src/promql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,12 @@ fn walk(expr: &Expr) -> Result<L2> {
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(),
Expand Down Expand Up @@ -413,8 +417,8 @@ fn walk_histogram_quantile(call: &Call) -> Result<L2> {
}

fn walk_binary(bin: &BinaryExpr) -> Result<L2> {
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 {
Expand Down Expand Up @@ -881,13 +885,47 @@ fn num_param(agg: &AggregateExpr) -> Result<f64> {
fn num_expr(expr: &Expr) -> Result<f64> {
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<L2> {
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, …)`).
Expand Down
25 changes: 14 additions & 11 deletions crates/frontend-promql/tests/awesome_prometheus_alerts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ fn intents(e: &QueryExpr) -> Vec<AggIntent> {
go(expr, out);
go(child, out);
}
QueryExpr::Scan { .. } | QueryExpr::Ref { .. } => {}
QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::Ref { .. } => {}
}
}
go(e, &mut out);
Expand Down Expand Up @@ -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 `<vector> <cmp> <scalar>` 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:?}"
);
}
Expand Down Expand Up @@ -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 `<vector> <cmp> <scalar>`. 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:?}"
);
}
}
Expand Down
35 changes: 29 additions & 6 deletions crates/frontend-promql/tests/promql_conformance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ fn collect(e: &QueryExpr, out: &mut Vec<AggIntent>) {
collect(expr, out);
collect(child, out);
}
QueryExpr::Scan { .. } | QueryExpr::Ref { .. } => {}
QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::Ref { .. } => {}
}
}

Expand Down Expand Up @@ -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: `<vector> op <scalar>` — 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));
}

// ─────────────────────────────────────────────────────────────────────────────
Expand Down
2 changes: 1 addition & 1 deletion crates/frontend-sql/tests/synthetic_packet_trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ fn intents(e: &QueryExpr) -> Vec<AggIntent> {
go(expr, out);
go(child, out);
}
QueryExpr::Scan { .. } | QueryExpr::Ref { .. } => {}
QueryExpr::Scan { .. } | QueryExpr::Scalar(_) | QueryExpr::Ref { .. } => {}
}
}
go(e, &mut out);
Expand Down
22 changes: 21 additions & 1 deletion crates/ir/src/intent_algebra/query_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<vector> op <scalar>` thresholds / unit conversions (#35).
Scalar(f64),

/// σ — row-level filter. Output schema = child schema.
Filter {
pred: Predicate,
Expand Down Expand Up @@ -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 `<vector> op <scalar>` (or `<scalar> op
// <vector>`) 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),
},
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions crates/l2/src/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
},
Expand Down
8 changes: 6 additions & 2 deletions crates/l2/src/relational.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<vector> op <scalar>` 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).
Expand Down Expand Up @@ -254,7 +258,7 @@ impl QueryExpr {
pub fn walk<F: FnMut(&QueryExpr)>(&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, .. }
Expand Down Expand Up @@ -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,
}
}

Expand Down
Loading