diff --git a/crates/e2e/tests/nested.rs b/crates/e2e/tests/nested.rs index bec96c4a..aaec15b9 100644 --- a/crates/e2e/tests/nested.rs +++ b/crates/e2e/tests/nested.rs @@ -175,6 +175,33 @@ fn q27_max_over_sum_by_job_over_rate() { ); } +// #53 — outer group key provably absent from the nested aggregate's output. +// The inner `sum by (group)` freezes the schema to the closed `[group, sum]`, +// which lacks `job`; PromQL groups every series under the empty label value +// and omits it from the output, so the outer `by (job)` lowers as a global +// aggregate (the absent key is dropped, not rejected). +// Scan schema: [ts(0), value(1), group(2), job(3)] (labels alphabetical). +#[test] +fn q53_outer_group_key_absent_from_nested_aggregate() { + let scan = QueryExpr::Scan { + source: Source::TimeSeries { + metric: "http_requests".into(), + }, + predicates: vec![Predicate(L3Expr::Compare { + left: Box::new(L3Expr::Column(3)), + op: CompareOp::Eq, + right: Box::new(L3Expr::Literal(L3Scalar::Utf8("api-server".into()))), + })], + schema: metric_schema(&["group", "job"]), + }; + let inner = agg(vec![2], AggIntent::Sum { col: None }, scan); + let expected = agg(vec![], AggIntent::Sum { col: None }, inner); + assert_eq!( + lower(r#"sum(sum by (group)(http_requests{job="api-server"})) by (job)"#), + 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 @@ -205,3 +232,50 @@ fn q24_sum_by_job_over_rate_over_filtered_scan() { expected, ); } + +// #27 — the nested sub-query example from the Prometheus docs +// (https://prometheus.io/docs/prometheus/latest/querying/examples/): +// max_over_time(deriv(rate(distance_covered_total[5s])[30s:5s])[10m:]) +// Two stacked sub-queries feeding range functions; the outer `[10m:]` uses +// the default resolution (None). Every level is a per-series reduction, so +// the whole spine survives verbatim and the schema stays label-preserving. +#[test] +fn q27_nested_subquery_prometheus_docs_example() { + let scan = QueryExpr::Scan { + source: Source::TimeSeries { + metric: "distance_covered_total".into(), + }, + predicates: vec![], + schema: metric_schema(&[]), + }; + let rate = agg( + vec![], + AggIntent::Rate, + QueryExpr::TimeRange { + range: Duration::from_secs(5), + child: Box::new(scan), + }, + ); + let deriv = agg( + vec![], + AggIntent::Deriv, + QueryExpr::Subquery { + range: Duration::from_secs(30), + resolution: Some(Duration::from_secs(5)), + child: Box::new(rate), + }, + ); + let expected = agg( + vec![], + AggIntent::Max { col: None }, + QueryExpr::Subquery { + range: Duration::from_secs(600), + resolution: None, + child: Box::new(deriv), + }, + ); + assert_eq!( + lower("max_over_time(deriv(rate(distance_covered_total[5s])[30s:5s])[10m:])"), + expected, + ); +} diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index 5d3be208..21dfbfdb 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -668,6 +668,53 @@ fn outer_aggregate_over_nested_aggregate_nests() { assert!(has(&qe, |i| matches!(i, AggIntent::Rate)), "rate preserved"); } +#[test] +fn outer_group_key_absent_from_nested_aggregate_is_dropped() { + // SEMANTICS (PromQL, issue #53): aggregating `by` a label that no input + // series carries is valid — every series lands in one group and the + // (empty) label is omitted from the output. Here the inner `sum by (group)` + // collapses `job` away (its closed output schema is `[group, sum]`), so the + // outer `by (job)` groups everything into a single global partition: + // the query lowers with the provably-absent key dropped, exactly + // `sum(sum by (group)(…))`. + let qe = ok(r#"sum(sum by (group)(http_requests{job="api-server"})) by (job)"#); + let QueryExpr::Aggregate { + by, aggs, child, .. + } = &qe + else { + panic!("expected outer Aggregate, got {qe:?}"); + }; + assert!(by.is_empty(), "absent `job` key dropped → global aggregate"); + assert!(matches!(aggs.as_slice(), [AggIntent::Sum { .. }])); + let QueryExpr::Aggregate { by, .. } = child.as_ref() else { + panic!("expected inner `sum by (group)` Aggregate, got {child:?}"); + }; + assert_eq!(by, &vec![2], "inner grouping on `group` survives"); +} + +#[test] +fn outer_group_key_present_after_inner_aggregate_still_resolves() { + // The counterpart guard for #53: when the outer key IS in the inner + // aggregate's output (`by (job)` over `sum by (job, group)`), it must keep + // resolving positionally — the absent-key drop only fires on provable + // absence, never on a resolvable key. + let qe = ok("sum(sum by (job, group)(http_requests)) by (job)"); + let QueryExpr::Aggregate { by, child, .. } = &qe else { + panic!("expected outer Aggregate, got {qe:?}"); + }; + let QueryExpr::Aggregate { by: inner_by, .. } = child.as_ref() else { + panic!("expected inner Aggregate, got {child:?}"); + }; + // Inner output schema is [group, job, sum] (keys in label-column order, + // labels alphabetical on the scan) → job = col 1. + assert_eq!( + by, + &vec![1], + "outer `job` resolves against the inner output" + ); + assert_eq!(inner_by.len(), 2); +} + #[test] fn aggregate_over_binary_op_nests() { // `sum(rate(a[5m]) + rate(b[5m]))` — an aggregate whose argument is a binary @@ -752,6 +799,66 @@ fn aggregation_over_over_time_of_subquery_keeps_labels() { assert!(matches!(inner_child.as_ref(), QueryExpr::Subquery { .. })); } +#[test] +fn nested_subquery_from_prometheus_docs() { + // SEMANTICS (PromQL): the *nested sub-query* example from the official docs + // (): + // + // max_over_time(deriv(rate(distance_covered_total[5s])[30s:5s])[10m:]) + // + // Two stacked sub-queries, each feeding a range-vector function; the outer + // `[10m:]` uses the **default resolution** (no explicit step). Each level + // lowers to its own node, so the whole spine pins as: + // + // Max ∘ Subquery{10m, res: None} ∘ Deriv ∘ Subquery{30s, res: 5s} + // ∘ Rate ∘ TimeRange{5s} ∘ Scan + // + // Every reduction is per-series (no grouping), so the output schema stays + // the label-preserving `[ts, value]`. + let qe = ok("max_over_time(deriv(rate(distance_covered_total[5s])[30s:5s])[10m:])"); + + let QueryExpr::Aggregate { by, aggs, child, .. } = &qe else { + panic!("expected `max_over_time` Aggregate at the root, got {qe:?}"); + }; + assert!(by.is_empty()); + assert!(matches!(aggs.as_slice(), [AggIntent::Max { .. }])); + + let QueryExpr::Subquery { range, resolution, child } = child.as_ref() else { + panic!("expected the outer `[10m:]` Subquery, got {child:?}"); + }; + assert_eq!(*range, Duration::from_secs(600)); + assert_eq!(*resolution, None, "`[10m:]` keeps the default resolution"); + + let QueryExpr::Aggregate { by, aggs, child, .. } = child.as_ref() else { + panic!("expected the `deriv` Aggregate, got {child:?}"); + }; + assert!(by.is_empty()); + assert!(matches!(aggs.as_slice(), [AggIntent::Deriv])); + + let QueryExpr::Subquery { range, resolution, child } = child.as_ref() else { + panic!("expected the inner `[30s:5s]` Subquery, got {child:?}"); + }; + assert_eq!(*range, Duration::from_secs(30)); + assert_eq!(*resolution, Some(Duration::from_secs(5))); + + let QueryExpr::Aggregate { aggs, child, .. } = child.as_ref() else { + panic!("expected the `rate` Aggregate, got {child:?}"); + }; + assert!(matches!(aggs.as_slice(), [AggIntent::Rate])); + let QueryExpr::TimeRange { range, .. } = child.as_ref() else { + panic!("expected the `[5s]` TimeRange under rate, got {child:?}"); + }; + assert_eq!(*range, Duration::from_secs(5)); + + // Per-series end to end: the schema keeps the (ts, value) floor and stays open. + let schema = qe.output_schema().expect("schema derivation"); + assert_eq!( + schema.columns.iter().map(|c| c.name.as_str()).collect::>(), + vec!["ts", "value"], + ); + assert!(!schema.closed, "per-series chain never freezes the schema"); +} + // ───────────────────────────────────────────────────────────────────────────── // K. Time-shift modifiers (basics §Offset/@; at_modifier.test) // ───────────────────────────────────────────────────────────────────────────── diff --git a/crates/frontend-sql/tests/sql_lowering.rs b/crates/frontend-sql/tests/sql_lowering.rs index 15db83cc..7ac123fc 100644 --- a/crates/frontend-sql/tests/sql_lowering.rs +++ b/crates/frontend-sql/tests/sql_lowering.rs @@ -697,3 +697,24 @@ async fn scalar_subquery_in_predicate_is_rejected() { "scalar subquery in predicate should be rejected" ); } + +#[tokio::test] +async fn exists_subquery_in_predicate_is_rejected() { + // The v1 decision on #27's predicate-subquery question: **reject cleanly**, + // for the whole family — scalar (above), IN (the semi-join test), and + // EXISTS / NOT EXISTS / NOT IN here, correlated or not. Nothing mislowers: + // the subquery predicate is never silently dropped. + for q in [ + // Correlated EXISTS. + "SELECT service FROM metrics m WHERE EXISTS \ + (SELECT 1 FROM hosts h WHERE h.service = m.service)", + // NOT EXISTS (anti-join shape). + "SELECT service FROM metrics m WHERE NOT EXISTS \ + (SELECT 1 FROM hosts h WHERE h.service = m.service)", + // NOT IN (negated semi-join shape). + "SELECT service FROM metrics WHERE service NOT IN (SELECT service FROM hosts)", + ] { + let res = lower_sql(q, &catalog(), AccuracyTarget::Exact).await; + assert!(res.is_err(), "predicate subquery should be rejected: {q}"); + } +} diff --git a/crates/l2/src/column_resolution.rs b/crates/l2/src/column_resolution.rs index c96db7c5..b4ce90ed 100644 --- a/crates/l2/src/column_resolution.rs +++ b/crates/l2/src/column_resolution.rs @@ -101,6 +101,33 @@ pub fn resolve_column_refs( cols.iter().map(|c| resolve_column_ref(c, schema)).collect() } +/// Resolve PromQL aggregation grouping keys with the language's absent-label +/// semantics (issue #53): a key not present in a **closed** schema is provably +/// absent from every row, so all rows carry its empty value, grouping by it is +/// the identity partition, and Prometheus omits the (empty) label from the +/// aggregation output — so the key is **dropped** rather than rejected. This +/// is what makes `sum(sum by (k) (m)) by (j)` lower: the inner cross-series +/// aggregate freezes the schema to the closed `[k, sum]`, which provably lacks +/// `j`. +/// +/// Against an **open** schema an unresolved key is still an error: the label +/// may exist at runtime, and PromQL leaves seed every referenced label via the +/// Binder, so an unresolved key over an open schema indicates a resolution bug, +/// not an absent label. (SQL is unaffected — its `GROUP BY` resolves through +/// the strict [`resolve_column_refs`], and DataFusion has already validated +/// the columns anyway.) +pub fn resolve_group_keys_promql( + cols: &[ColumnRef], + schema: &Schema, +) -> Result, ResolveError> { + cols.iter() + .filter_map(|c| match resolve_column_ref(c, schema) { + Err(ResolveError::NotFound { .. }) if schema.closed => None, + other => Some(other), + }) + .collect() +} + /// Resolve a Layer-2 [`L2Expr`] (name-based) into a positional [`L3Expr`] by /// resolving every column reference against `schema`. Structural otherwise. pub fn resolve_expr(expr: &L2Expr, schema: &Schema) -> Result { @@ -250,6 +277,48 @@ mod tests { assert!(matches!(err, ResolveError::NotFound { .. })); } + #[test] + fn group_keys_promql_drops_absent_key_in_closed_schema() { + // The output of a nested cross-series aggregate: closed `[group, sum]`. + // `by (job)` — `job` is provably absent → dropped, not rejected (#53). + let s = Schema { + columns: vec![ + Column::new("group", DataType::Utf8, true), + Column::new("sum", DataType::Float64, false), + ], + time_index: None, + unique_keys: vec![], + closed: true, + }; + assert_eq!( + resolve_group_keys_promql(&[ColumnRef::Named("job".into())], &s), + Ok(vec![]) + ); + // Present keys still resolve positionally; absent ones drop around them. + assert_eq!( + resolve_group_keys_promql( + &[ + ColumnRef::Named("job".into()), + ColumnRef::Named("group".into()) + ], + &s + ), + Ok(vec![0]) + ); + } + + #[test] + fn group_keys_promql_still_errors_on_open_schema() { + // An open schema can't prove absence — an unresolved key there is a + // resolution bug (the Binder seeds every referenced label), not an + // absent label. Keep the strict error. + let open = infer_source_schema("m"); // closed: false + assert!(matches!( + resolve_group_keys_promql(&[ColumnRef::Named("job".into())], &open), + Err(ResolveError::NotFound { .. }) + )); + } + #[test] fn aggregate_strips_time_and_keeps_unique_keys() { let mut input = infer_source_schema("m"); diff --git a/crates/l2/src/lower.rs b/crates/l2/src/lower.rs index ca6297ef..8c0f71b5 100644 --- a/crates/l2/src/lower.rs +++ b/crates/l2/src/lower.rs @@ -17,7 +17,8 @@ use thiserror::Error; use asap_ir::intent_algebra::agg_intent::AggIntent; use crate::binder::Binder; use crate::column_resolution::{ - output_schema_for_aggregate, resolve_column_refs, resolve_expr, ResolveError, + output_schema_for_aggregate, resolve_column_refs, resolve_expr, resolve_group_keys_promql, + ResolveError, }; use asap_ir::intent_algebra::expr_ir::{ColumnRef, L2Expr, L3Expr, L3Scalar}; use asap_ir::intent_algebra::names::BindingName; @@ -262,7 +263,12 @@ pub fn convert( if time_range.is_some() && !keys.is_empty() { return Err(ConvertError::WindowedReductionKeys); } - let by: GroupKeys = resolve_column_refs(keys, &agg_in_schema)?.into(); + // PromQL absent-label grouping semantics (issue #53): a key + // provably absent from a *closed* input schema (e.g. the output + // of a nested cross-series aggregate that collapsed the label) + // groups every series into one partition and is omitted from + // the output — drop it instead of rejecting the query. + let by: GroupKeys = resolve_group_keys_promql(keys, &agg_in_schema)?.into(); return Ok(CQueryExpr::Aggregate { by, aggs: vec![intent], diff --git a/docs/promql-lowering.md b/docs/promql-lowering.md index be883ca6..f5508d9a 100644 --- a/docs/promql-lowering.md +++ b/docs/promql-lowering.md @@ -152,6 +152,44 @@ plans never read `unique_keys`. --- +## The nesting contract (issue #27) + +Nesting is **structural** at L3: `QueryExpr` is a recursive, box-owned tree and +every operator accepts an arbitrary child, so "function over subquery" needs no +special IR. The contract below says which *composite arguments* the front ends +actually lower today, and which are **cleanly rejected** (never silently +mislowered) — each row is pinned by a named test. + +### Lowers + +| Shape | Example | Pinned by | +|---|---|---| +| Aggregate over aggregate (any depth) | `max(sum by (job) (rate(m[5m])))` | `outer_aggregate_over_nested_aggregate_nests`; e2e `q27_max_over_sum_by_job_over_rate` | +| Aggregate over a binary op | `sum(rate(a[5m]) + rate(b[5m]))` | `aggregate_over_binary_op_nests` | +| Binary op over two arbitrary subtrees | `sum by (j)(rate(a[5m])) / sum by (j)(rate(b[5m]))` | e2e `q25_div_over_complex_subtrees` | +| Ranking over a nested aggregate (generic top-k) | `topk(3, sum by (i)(rate(m[5m])))` → `Sort → Limit` | `topk_over_nested_aggregate_is_generic_sort_limit` | +| Heavy-hitter top-k (frequency shape) | `topk(10, count_over_time(m[1m]))` → `AggIntent::TopK` | `topk_over_count_is_heavy_hitter` | +| Range function over a sub-query (#42) | `max_over_time(rate(m[5m])[1h:])` | `over_time_of_subquery_reduces_per_series` | +| Nested sub-queries (range fn over range fn over range fn; default resolution) | `max_over_time(deriv(rate(distance_covered_total[5s])[30s:5s])[10m:])` (Prometheus docs example) | `nested_subquery_from_prometheus_docs`; e2e `q27_nested_subquery_prometheus_docs_example` | +| Outer group key absent from a nested aggregate's (closed) output (#53) | `sum(sum by (k)(m)) by (j)` — `j` provably absent → dropped per PromQL's absent-label grouping semantics | `outer_group_key_absent_from_nested_aggregate_is_dropped`; e2e `q53_outer_group_key_absent_from_nested_aggregate` | +| SQL derived tables / inline views (#29) | `SELECT … FROM (SELECT … GROUP BY …) t`, incl. aggregate-over-aggregate | `derived_table_aggregate_over_aggregate_nests` (`sql_lowering.rs`) | + +### Rejected cleanly + +| Shape | Why | Pinned by | +|---|---|---| +| Unary negation anywhere in a nest — `sum(-m)` | no scalar-negate in the L2 PromQL path yet (issue #36) | `unary_negation_is_rejected__GAP` | +| `without(...)` grouping | a usage-derived (open) schema can't enumerate the label complement (issue #39) | binder rejection (see "Why the Binder is its own pass") | +| SQL subquery-valued **predicate** expressions — scalar `x > (SELECT …)`, `IN (SELECT …)`, `EXISTS` / `NOT EXISTS` / `NOT IN`, correlated or not | the v1 decision on #27's open question: these need a subquery node in the L2 expression IR + a correlated-vs-uncorrelated representation choice; rejected until that lands (derived tables in `FROM` are the supported nesting shape) | `scalar_subquery_in_predicate_is_rejected`, `semi_join_is_rejected_not_mislowered`, `exists_subquery_in_predicate_is_rejected` (`sql_lowering.rs`) | + +The dividing line: **relational** nesting (an operator tree in `FROM` / a +function argument) lowers structurally; **expression-level** subqueries (a +query embedded inside a scalar predicate) are the one shape with no L2/L3 +representation yet, and they are the only nesting rejections that are not +tracked by a more specific issue. + +--- + ## Worked example — one query through L1 → L2 → L3 Four steps: the three layers, plus the Binder pass shown explicitly on the