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
74 changes: 74 additions & 0 deletions crates/e2e/tests/nested.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
);
}
107 changes: 107 additions & 0 deletions crates/frontend-promql/tests/promql_conformance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
// (<https://prometheus.io/docs/prometheus/latest/querying/examples/>):
//
// 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<_>>(),
vec!["ts", "value"],
);
assert!(!schema.closed, "per-series chain never freezes the schema");
}

// ─────────────────────────────────────────────────────────────────────────────
// K. Time-shift modifiers (basics §Offset/@; at_modifier.test)
// ─────────────────────────────────────────────────────────────────────────────
Expand Down
21 changes: 21 additions & 0 deletions crates/frontend-sql/tests/sql_lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}");
}
}
69 changes: 69 additions & 0 deletions crates/l2/src/column_resolution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<ColumnId>, 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<L3Expr, ResolveError> {
Expand Down Expand Up @@ -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");
Expand Down
10 changes: 8 additions & 2 deletions crates/l2/src/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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],
Expand Down
38 changes: 38 additions & 0 deletions docs/promql-lowering.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading