diff --git a/crates/e2e/tests/nested.rs b/crates/e2e/tests/nested.rs index aaec15b9..a252bd2f 100644 --- a/crates/e2e/tests/nested.rs +++ b/crates/e2e/tests/nested.rs @@ -12,6 +12,7 @@ use std::time::Duration; use asap_ir::intent_algebra::{ AggIntent, ArithOp, BinaryOpKind, CompareOp, L3Expr, L3Scalar, Predicate, QueryExpr, Source, + VectorMatch, VectorMatchKind, }; use asap_ir::types::AccuracyTarget; use asap_frontend_promql::lower_promql; @@ -202,6 +203,46 @@ fn q53_outer_group_key_absent_from_nested_aggregate() { ); } +// #52 — an outer group key referenced by neither binary-op side (`__name__`) +// still resolves. Each `or` side is bound independently against its own +// sub-tree, so `__name__` is seeded as an inherited column on both. Each side +// references only `env` (its matcher), so its schema is [ts, value, env, +// __name__] (referenced `env` first, inherited `__name__` appended) → the +// outer `by (__name__)` resolves to col 3 on both sides. The `or` carries the +// parser's default `ignoring([])` match modifier. +#[test] +fn q52_outer_name_label_over_binary_op() { + let side = |metric: &str, env: &str| QueryExpr::Scan { + source: Source::TimeSeries { + metric: metric.into(), + }, + predicates: vec![Predicate(L3Expr::Compare { + left: Box::new(L3Expr::Column(2)), // env + op: CompareOp::Eq, + right: Box::new(L3Expr::Literal(L3Scalar::Utf8(env.into()))), + })], + schema: metric_schema(&["env", "__name__"]), + }; + let expected = agg( + vec![3], // __name__ + AggIntent::Sum { col: None }, + QueryExpr::BinaryOp { + op: BinaryOpKind::Or, + lhs: Box::new(side("metric_a", "1")), + rhs: Box::new(side("metric_b", "2")), + vector_match: Some(VectorMatch { + kind: VectorMatchKind::Ignoring, + labels: vec![], + grouping: None, + }), + }, + ); + assert_eq!( + lower(r#"sum by (__name__)(metric_a{env="1"} or metric_b{env="2"})"#), + expected, + ); +} + // #24 — sum by job over rate over a filtered scan // same schema [ts, value, job, status]; rate is label-preserving, // so outer sum by job still finds job at col 2 diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index 088d9c94..44c2c8b0 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -790,6 +790,34 @@ fn outer_group_key_present_after_inner_aggregate_still_resolves() { assert_eq!(inner_by.len(), 2); } +#[test] +fn outer_group_key_over_binary_op_resolves_on_both_sides() { + // Issue #52: an outer aggregate's group key that appears in *neither* side of + // a binary op — the metric-name label `__name__`, or a plain `job` — must + // still resolve. Each `or` side is bound independently against its own + // sub-tree, so the key is seeded as an inherited column on both sides. + let qe = ok(r#"sum by (__name__)(metric_a{env="1"} or metric_b{env="2"})"#); + let QueryExpr::Aggregate { by, child, .. } = &qe else { + panic!("expected outer Aggregate, got {qe:?}"); + }; + // `__name__` resolves to a single positional id against the binary op output. + assert_eq!(by.len(), 1, "grouped by the one `__name__` key"); + let QueryExpr::BinaryOp { lhs, rhs, .. } = child.as_ref() else { + panic!("expected a BinaryOp child, got {child:?}"); + }; + // Both independently-bound sides carry `__name__` at the same position, so + // the outer group key is consistent across the union. + let (ls, rs) = (lhs.output_schema().unwrap(), rhs.output_schema().unwrap()); + assert_eq!(ls.column_id("__name__"), rs.column_id("__name__")); + assert_eq!(ls.column_id("__name__"), Some(by[0])); + + // The general case (a plain label, not just `__name__`) also lowers. + assert!(matches!( + ok("sum by (job)(metric_a or metric_b)"), + QueryExpr::Aggregate { .. } + )); +} + #[test] fn aggregate_over_binary_op_nests() { // `sum(rate(a[5m]) + rate(b[5m]))` — an aggregate whose argument is a binary diff --git a/crates/l2/src/binder.rs b/crates/l2/src/binder.rs index e9ab8432..459fcf20 100644 --- a/crates/l2/src/binder.rs +++ b/crates/l2/src/binder.rs @@ -73,6 +73,16 @@ impl Binder { /// per distinct name referenced anywhere in the tree — so positional /// `ColumnId` resolution downstream is total. pub fn bind(&self, tree: &LQueryExpr) -> Schema { + self.bind_with_inherited(tree, &[]) + } + + /// Like [`bind`](Self::bind), but also seeds `inherited` label names that are + /// referenced by an **enclosing** scope rather than by `tree` itself. This is + /// how an independently-bound `BinaryOp` side (each side re-binds against its + /// own sub-tree) still sees an outer aggregate's group keys — e.g. the + /// `__name__` / `job` in `sum by (__name__)(a or b)`, which appear in neither + /// side's own matchers (issue #52). + pub fn bind_with_inherited(&self, tree: &LQueryExpr, inherited: &[String]) -> Schema { let mut columns: Vec = tree .source_name() .and_then(|name| self.catalog.columns_for(name)) @@ -85,10 +95,12 @@ impl Binder { } } - // Append one column per referenced-but-unknown name (group keys etc.). - for name in collect_referenced_columns(tree) { - if !columns.iter().any(|c| c.name == name) { - columns.push(Column::new(name, DataType::Utf8, true)); + // Append one column per referenced-but-unknown name (group keys etc.), + // plus any inherited-from-enclosing-scope names. + let referenced = collect_referenced_columns(tree); + for name in referenced.iter().chain(inherited) { + if !columns.iter().any(|c| c.name == *name) { + columns.push(Column::new(name.clone(), DataType::Utf8, true)); } } @@ -128,7 +140,7 @@ fn push_ref_name(c: &ColumnRef, out: &mut Vec) { } } -fn collect_referenced_columns(tree: &LQueryExpr) -> Vec { +pub(crate) fn collect_referenced_columns(tree: &LQueryExpr) -> Vec { fn named(expr: &L2Expr, out: &mut Vec) { for c in expr.columns_referenced() { push_ref_name(c, out); @@ -211,6 +223,18 @@ mod tests { assert!(schema.column_id("host").is_some()); } + #[test] + fn inherited_names_are_seeded_alongside_referenced() { + // A `BinaryOp` side re-binds against its own sub-tree, but must still see + // an enclosing aggregate's group key (`__name__` / `job`) that appears in + // neither side's own matchers (issue #52). `bind_with_inherited` seeds it. + let schema = Binder::new().bind_with_inherited(&src("m"), &["__name__".into()]); + assert!(schema.column_id("__name__").is_some()); + // `bind` (no inheritance) does not conjure it. + let plain = Binder::new().bind(&src("m")); + assert!(plain.column_id("__name__").is_none()); + } + #[test] fn custom_catalog_supplies_base_columns() { struct FixedCatalog; diff --git a/crates/l2/src/lower.rs b/crates/l2/src/lower.rs index 8c0f71b5..3c21b3f0 100644 --- a/crates/l2/src/lower.rs +++ b/crates/l2/src/lower.rs @@ -15,7 +15,7 @@ use std::time::Duration; use thiserror::Error; use asap_ir::intent_algebra::agg_intent::AggIntent; -use crate::binder::Binder; +use crate::binder::{collect_referenced_columns, Binder}; use crate::column_resolution::{ output_schema_for_aggregate, resolve_column_refs, resolve_expr, resolve_group_keys_promql, ResolveError, @@ -63,7 +63,19 @@ pub fn convert_root( legacy: &LQueryExpr, accuracy: &AccuracyTarget, ) -> Result { - let fallback = Binder::new().bind(legacy); + convert_root_with_inherited(legacy, accuracy, &[]) +} + +/// [`convert_root`] with label names inherited from an enclosing scope seeded +/// into the leaf schema. Used when re-binding a `BinaryOp` side, so an outer +/// aggregate's group keys (`sum by (__name__)(a or b)`) resolve even though they +/// appear in neither side's own sub-tree (issue #52). +fn convert_root_with_inherited( + legacy: &LQueryExpr, + accuracy: &AccuracyTarget, + inherited: &[String], +) -> Result { + let fallback = Binder::new().bind_with_inherited(legacy, inherited); let l3 = convert(legacy, &fallback, accuracy)?; // Both language front ends end here, so this is the one place to normalize // structural differences between equivalent queries (issue #34). @@ -511,20 +523,48 @@ pub fn convert( lhs, rhs, vector_match, - } => CQueryExpr::BinaryOp { - op: op.clone(), + } => { // A binary op's two sides may scan different metrics with different // label sets, so each branch must resolve against its OWN bound - // schema. `convert_root` re-runs the Binder per sub-tree; threading - // the parent `schema` (derived from the left leaf only) would bind - // the right side's columns to the wrong positions. - lhs: Box::new(convert_root(lhs, acc)?), - rhs: Box::new(convert_root(rhs, acc)?), - vector_match: vector_match.clone(), - }, + // schema — `convert_root` re-runs the Binder per sub-tree; threading + // the parent `schema` (a superset over both leaves) would bind the + // right side's columns to the wrong positions. + // + // But an independently-bound side still has to see label names an + // *enclosing* node references — e.g. an outer `sum by (__name__)` / + // `sum by (job)` over `(a or b)` whose key is in neither side's own + // matchers (issue #52). `fallback` carries every name referenced in + // this scope; subtract the names referenced *within* the binary op + // itself and what remains is exactly the ancestor-referenced set to + // inherit — seeding a sibling side's own labels would wrongly leak + // them across the two branches. + let own = collect_referenced_columns(legacy); + let inherited: Vec = inherited_names(fallback) + .into_iter() + .filter(|n| !own.contains(n)) + .collect(); + CQueryExpr::BinaryOp { + op: op.clone(), + lhs: Box::new(convert_root_with_inherited(lhs, acc, &inherited)?), + rhs: Box::new(convert_root_with_inherited(rhs, acc, &inherited)?), + vector_match: vector_match.clone(), + } + } }) } +/// The label names an enclosing scope's schema carries beyond the `(ts, value)` +/// floor — the set an independently-bound `BinaryOp` side must inherit so an +/// outer aggregate's group keys still resolve (issue #52). +fn inherited_names(schema: &Schema) -> Vec { + schema + .columns + .iter() + .filter(|c| c.name != "ts" && c.name != "value") + .map(|c| c.name.clone()) + .collect() +} + /// Build a canonical `Scan`. A schema-bearing [`SourceSpec`] (SQL table) emits /// a `Source::Table` carrying that resolved schema; a schema-less one (PromQL) /// emits a `Source::TimeSeries` carrying the Binder's usage-derived `fallback`. diff --git a/docs/promql-lowering.md b/docs/promql-lowering.md index 5ce91ed8..e8f464b0 100644 --- a/docs/promql-lowering.md +++ b/docs/promql-lowering.md @@ -108,6 +108,20 @@ Why isolate this instead of resolving inline during lowering: rejected here: a usage-derived schema can't enumerate "all labels *except* these," so the error belongs in binding, not smeared across lowering. +### Binary-op sides bind independently — with inherited keys + +Each side of a `BinaryOp` re-runs the Binder against its **own** sub-tree +(`convert_root` per side), because the two branches may scan different metrics +with different label sets and each side's columns must bind to positions in its +own leaf — not the other's. But a side must still see label names referenced by +an **enclosing** node: `sum by (__name__)(a or b)` (or `sum by (job)(a or b)`) +groups by a key that appears in *neither* side's own matchers, and every series +carries `__name__` (the metric name) regardless (issue #52). So the converter +seeds those **inherited** names — the enclosing scope's referenced columns minus +the ones referenced inside the binary op itself, i.e. exactly the ancestor keys — +into each side via `Binder::bind_with_inherited`. Subtracting the binary op's own +references is what keeps one side's labels from leaking into the other. + --- ## Why `unique_keys` / CSE