From 61dad0d3831468fded96ce398f22b987aa3a636a Mon Sep 17 00:00:00 2001 From: zz_y Date: Fri, 3 Jul 2026 12:51:40 -0600 Subject: [PATCH 1/2] feat(promql): lower group() and count_values() aggregation operators (#49) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `group`, `count_values`, `limitk`, `limit_ratio` all parsed but were rejected as UnsupportedAggregateOp. Add the two genuine *aggregation* operators; the sampling operators are split to #86 (they need a selection node, not an aggregate intent). L3 (asap-ir): - `AggIntent::Group` — constant 1 per group ("group presence"). Distinct from Sum/Count (its value is always 1, independent of inputs); previously `group` was mislowered onto `Sum`. - `AggIntent::CountValues { label }` — group the input series by their sample value, count each distinct value, and emit the value as a new label. Unlike every other reducer it adds a synthesized Utf8 label column, so `Aggregate` schema derivation special-cases it (two output columns: the label + count), and drops the by-keys-only unique key. Both added to `agg_is_exact`. L2 (asap-l2): - `AggFunc::Group` / `AggFunc::CountValues { label }` + converter arms. Front end (asap-frontend-promql): - `T_GROUP` → `OuterIntent::Group`; `T_COUNT_VALUES` → `Outer::CountValues` carrying the string label parameter (`str_param`, which unwraps nested parens: `count_values((("v")), m)`). Both flow through the existing two-level `build` / `build_over_subtree` machinery. `limitk`/`limit_ratio` stay rejected: they are series-*sampling* selection (return an unordered subset of series unchanged), so modeling them as topk would change which series pass through. Split to #86, pinned by `limitk_and_limit_ratio_are_rejected__GAP`. Tests: conformance §S (group intent + keys, count_values value→label + count, parenthesised label + by-grouping, limitk/limit_ratio GAP). Flipped the old `group_aggregator_is_rejected` pin and the `group_is_not_sum` equivalence pin (reject → distinct). Co-Authored-By: Claude Opus 4.8 --- crates/frontend-promql/src/promql.rs | 55 ++++++++++-- .../tests/promql_conformance.rs | 86 ++++++++++++++++++- .../tests/promql_equivalence.rs | 7 +- crates/ir/src/intent_algebra/agg_intent.rs | 25 ++++++ crates/ir/src/intent_algebra/query_expr.rs | 20 ++++- crates/l2/src/lower.rs | 4 + crates/l2/src/relational.rs | 6 ++ 7 files changed, 191 insertions(+), 12 deletions(-) diff --git a/crates/frontend-promql/src/promql.rs b/crates/frontend-promql/src/promql.rs index aa580806..043e910e 100644 --- a/crates/frontend-promql/src/promql.rs +++ b/crates/frontend-promql/src/promql.rs @@ -33,6 +33,7 @@ //! | `group` / `offset` / `@` / `info` | **rejected** — distinct semantics with no intent-algebra representation yet (`info` label-join → #84) | //! | `OUTER by (dims) (…)` | `Aggregate.keys = dims` (→ positional `Aggregate.by` in L3; generic `topk by`/`bottomk` grouping → `Sort.partition_by`) | //! | `count by (d) (…)` | `Aggregate{[CountDistinct], …}` (→ `Cardinality`) | +//! | `group(v)` / `count_values("l", v)` | `Aggregate{[Group]}` (constant 1) / `Aggregate{[CountValues{l}]}` (group-by-value + count, new label `l`) — issue #49; `limitk`/`limit_ratio` series sampling → #86 | //! | `topk(k, count_over_time(…))` | `TopK{k, by}` (heavy-hitter intent) | //! | `topk(k, )` / `bottomk(k, …)` | `Sort{value} → Limit{k}` | //! | `m{f}` | `Filter(Source)` | @@ -68,6 +69,9 @@ enum Outer { None, Plain(OuterIntent), Count, + /// `count_values("l", v)` — group by value + count, emitting the value as a + /// new label `l` (issue #49). + CountValues { label: String }, TopK { k: u64, descending: bool }, } @@ -80,6 +84,8 @@ enum OuterIntent { StdDev, Variance, Quantile(f64), + /// `group(v)` — constant 1 per group (issue #49). + Group, } #[derive(Debug, Clone)] @@ -355,11 +361,14 @@ fn outer_kind(agg: &AggregateExpr) -> Result { Outer::Plain(OuterIntent::Sum) } else if op == token::T_GROUP { // `group(v)` yields a constant 1 per group (presence), not a sum of - // values. Folding it onto `Sum` changed the result; reject until a - // distinct group-presence intent exists. - return Err(LoweringError::UnsupportedAggregateOp( - "`group` (constant-1 presence) is not `sum`; no distinct intent yet".into(), - )); + // values — a distinct intent, never folded onto `Sum` (issue #49). + Outer::Plain(OuterIntent::Group) + } else if op == token::T_COUNT_VALUES { + // `count_values("l", v)` groups by sample value and counts, emitting the + // value as a new label `l` (the string parameter) — issue #49. + Outer::CountValues { + label: str_param(agg)?, + } } else if op == token::T_AVG { Outer::Plain(OuterIntent::Avg) } else if op == token::T_MIN { @@ -394,6 +403,9 @@ fn build_over_subtree(outer: Outer, keys: Vec, child: L2) -> Result child, Outer::Plain(intent) => outer_aggregate(keys, outer_func(&intent), child), Outer::Count => outer_aggregate(keys, AggFunc::CountDistinct, child), + Outer::CountValues { label } => { + outer_aggregate(keys, AggFunc::CountValues { label }, child) + } Outer::TopK { k, descending } => { let sorted = L2::Sort { keys: vec![L2SortKey { @@ -849,6 +861,17 @@ fn build(inner: Inner, keys: Vec, outer: Outer) -> Result { outer_aggregate(keys, AggFunc::CountDistinct, inner_agg) } }), + Outer::CountValues { label } => { + let func = AggFunc::CountValues { label }; + Ok(match &inner.func { + None => windowed_aggregate(inner, keys, func), + Some(f) => { + let inner_f = inner_func(f); + let inner_agg = windowed_aggregate(inner, vec![], inner_f); + outer_aggregate(keys, func, inner_agg) + } + }) + } Outer::TopK { k, descending } => { // Heavy-hitter only when ranking by frequency (`count`): that is a // first-class aggregate intent → `TopK`. Any other ranking (topk @@ -999,6 +1022,28 @@ fn outer_func(o: &OuterIntent) -> AggFunc { OuterIntent::StdDev => AggFunc::StdDev { population: true }, OuterIntent::Variance => AggFunc::Variance { population: true }, OuterIntent::Quantile(q) => AggFunc::Quantile(*q), + OuterIntent::Group => AggFunc::Group, + } +} + +/// A `count_values` string parameter (the synthesized label name). PromQL wraps +/// it in a `StringLiteral`, possibly parenthesised (`count_values((("v")), …)`). +fn str_param(agg: &AggregateExpr) -> Result { + fn unwrap_str(expr: &Expr) -> Result { + match expr { + Expr::StringLiteral(s) => Ok(s.val.clone()), + Expr::Paren(p) => unwrap_str(&p.expr), + other => Err(LoweringError::InvalidParameter(format!( + "`count_values` label must be a string literal, got {:?}", + std::mem::discriminant(other) + ))), + } + } + match &agg.param { + Some(e) => unwrap_str(e), + None => Err(LoweringError::MissingArgument( + "`count_values` label parameter".into(), + )), } } diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index c2a6e00c..169849e6 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -310,10 +310,13 @@ fn sum_without_is_rejected() { } #[test] -fn group_aggregator_is_rejected() { +fn group_aggregator_lowers_to_a_distinct_intent() { // SEMANTICS (PromQL): `group(v)` returns a constant 1 per group (presence), - // NOT a sum. Rather than fold it onto `Sum` (wrong value) we reject it. - let _ = rejected("group by (job) (up)"); + // NOT a sum. It now lowers to a distinct `Group` intent (never folded onto + // `Sum`) — see §S. Regression guard that it is not a `Sum`. + let qe = ok("group by (job) (up)"); + assert!(has(&qe, |i| *i == AggIntent::Group)); + assert!(!has(&qe, |i| matches!(i, AggIntent::Sum { .. }))); } // ───────────────────────────────────────────────────────────────────────────── @@ -1278,3 +1281,80 @@ fn info_is_rejected__GAP() { // so adding support flips this deliberately. let _ = rejected("info(rate(http_requests_total[5m]))"); } + +// ───────────────────────────────────────────────────────────────────────────── +// S. Extended aggregation operators: group / count_values (aggregators.test; #49) +// ───────────────────────────────────────────────────────────────────────────── + +#[test] +fn group_lowers_to_a_constant_group_intent() { + // SEMANTICS: `group(v)` yields a constant 1 per group — a distinct intent, + // NOT folded onto `sum` (which would return the value sum instead of 1). + let qe = ok("group(up)"); + let QueryExpr::Aggregate { aggs, .. } = &qe else { + panic!("expected an Aggregate, got {qe:?}"); + }; + assert!(matches!(aggs.as_slice(), [AggIntent::Group])); + // Output column is the constant-1 `group` value. + let sch = qe.output_schema().unwrap(); + assert!(sch.columns.iter().any(|c| c.name == "group")); +} + +#[test] +fn group_by_keeps_the_grouping_keys() { + // `group by (job) (up)` — the grouping keys ride on `Aggregate.by`. + let qe = ok("group by (job) (up)"); + let sch = qe.output_schema().unwrap(); + assert!(sch.columns.iter().any(|c| c.name == "job")); + assert!(has(&qe, |i| *i == AggIntent::Group)); +} + +#[test] +fn count_values_groups_by_value_and_synthesizes_a_label() { + // SEMANTICS: `count_values("l", v)` groups the input series by their sample + // value, counts each distinct value, and emits that value as a new label + // `l`. The intent carries the label; schema gains a `Utf8` `l` column. + let qe = ok(r#"count_values("version", build_version)"#); + let QueryExpr::Aggregate { aggs, .. } = &qe else { + panic!("expected an Aggregate, got {qe:?}"); + }; + assert!( + matches!(aggs.as_slice(), [AggIntent::CountValues { label }] if label == "version") + ); + let sch = qe.output_schema().unwrap(); + let version = sch + .columns + .iter() + .find(|c| c.name == "version") + .expect("synthesized `version` label column"); + assert_eq!(version.dtype, DataType::Utf8, "the value becomes a string label"); + assert!( + sch.columns.iter().any(|c| c.name == "count"), + "and a count column" + ); +} + +#[test] +fn count_values_accepts_a_parenthesised_label_and_by_grouping() { + // `count_values by (job) ((("v")), m)` — nested parens around the string + // param, plus `by` grouping. Both survive. + let qe = ok(r#"count_values by (job) ((("v")), m)"#); + assert!(has( + &qe, + |i| matches!(i, AggIntent::CountValues { label } if label == "v") + )); + let sch = qe.output_schema().unwrap(); + assert!(sch.columns.iter().any(|c| c.name == "job")); + assert!(sch.columns.iter().any(|c| c.name == "v")); +} + +#[test] +fn limitk_and_limit_ratio_are_rejected__GAP() { + // `limitk`/`limit_ratio` are series-*sampling* operators — they return a + // deterministic-but-unordered subset of the input series unchanged. Modeling + // them as `topk` (order-by-value + limit) would change *which* series pass + // through, so they are cleanly rejected pending a sampling-selection node + // (follow-up #86), not mislowered. + let _ = rejected("limitk(2, http_requests)"); + let _ = rejected("limit_ratio(0.1, http_requests)"); +} diff --git a/crates/frontend-promql/tests/promql_equivalence.rs b/crates/frontend-promql/tests/promql_equivalence.rs index 1dc2255f..9702c3e2 100644 --- a/crates/frontend-promql/tests/promql_equivalence.rs +++ b/crates/frontend-promql/tests/promql_equivalence.rs @@ -174,9 +174,10 @@ fn changes_and_resets_are_not_count_over_time() { #[test] fn group_is_not_sum() { // PromQL `group` returns a constant 1 per group; it previously collapsed - // onto `sum` (sum of values). - assert_rejected("group(up)"); - assert_rejected("group by (job) (up)"); + // onto `sum` (sum of values). It now lowers to its own `Group` intent + // (issue #49) — distinct L3 from `sum`, not merged. + assert_distinct("group(up)", "sum(up)"); + assert_distinct("group by (job) (up)", "sum by (job) (up)"); } #[test] diff --git a/crates/ir/src/intent_algebra/agg_intent.rs b/crates/ir/src/intent_algebra/agg_intent.rs index 6c13d17e..ad2943d4 100644 --- a/crates/ir/src/intent_algebra/agg_intent.rs +++ b/crates/ir/src/intent_algebra/agg_intent.rs @@ -176,6 +176,23 @@ pub enum AggIntent { /// (`time()` is the evaluation time itself — a `QueryExpr::EvalTime` leaf, /// not this.) TimeFn(TimeFunc), + + // ── Extended aggregation operators (issue #49) ─────────────────────── + /// PromQL `group(v)` — a constant `1` per group ("group presence"). The + /// grouping keys ride on the enclosing `Aggregate.by`. Deliberately NOT + /// aliased to `Sum`/`Count`: the output value is always 1, independent of + /// the input values (folding it onto `Sum` would change the result). + Group, + /// PromQL `count_values("l", v)` — group the input series by their sample + /// *value* and count each distinct value, emitting that value as a new + /// label `l`. Output = one series per distinct value, with labels + /// `by-keys ∪ {l}` and value = the count. Unlike every other reducer this + /// adds a synthesized `Utf8` label column, so `Aggregate` schema derivation + /// special-cases it (two output columns, not one). + CountValues { + /// The name of the synthesized label carrying the stringified value. + label: String, + }, } /// Time / calendar accessor functions (issue #46), evaluated over a timestamp. @@ -363,6 +380,12 @@ impl AggIntent { AggIntent::AbsentOverTime => col("absent_over_time", DataType::Float64, false), AggIntent::PresentOverTime => col("present_over_time", DataType::Float64, false), AggIntent::TimeFn(_) => col("value", DataType::Float64, false), + // `group` — constant 1 per group. + AggIntent::Group => col("group", DataType::Float64, false), + // `count_values` — the *value* column (the per-value count). The + // synthesized `label` column is added alongside it by `Aggregate` + // schema derivation, which special-cases this intent. + AggIntent::CountValues { .. } => col("count", DataType::Int64, false), } } } @@ -403,6 +426,8 @@ pub fn agg_is_exact(op: &AggIntent) -> bool { | AggIntent::Avg { .. } | AggIntent::Min { .. } | AggIntent::Max { .. } + | AggIntent::Group + | AggIntent::CountValues { .. } ) } diff --git a/crates/ir/src/intent_algebra/query_expr.rs b/crates/ir/src/intent_algebra/query_expr.rs index 541b75cc..d12b3765 100644 --- a/crates/ir/src/intent_algebra/query_expr.rs +++ b/crates/ir/src/intent_algebra/query_expr.rs @@ -495,6 +495,19 @@ impl QueryExpr { // value probe (PromQL's single-column convention). A non-empty // `output_names[i]` overrides the synthetic output column name. for (i, intent) in aggs.iter().enumerate() { + // `count_values("l", v)` emits TWO columns: the synthesized + // `Utf8` label `l` (the stringified sample value it groups + // by) and the per-value count. `output_names[i]` still + // overrides the count column's name if set. + if let AggIntent::CountValues { label } = intent { + out_cols.push(Column::new(label.clone(), DataType::Utf8, false)); + let mut cnt = intent.output_column(&probe); + if let Some(name) = output_names.get(i).filter(|s| !s.is_empty()) { + cnt.name = name.clone(); + } + out_cols.push(cnt); + continue; + } let in_col = intent .input_col() .and_then(|id| in_schema.columns.get(id)) @@ -505,7 +518,12 @@ impl QueryExpr { } out_cols.push(out); } - let unique_keys = if by.is_empty() { + // `count_values` groups by (by-keys ∪ the synthesized value + // label), so the by-keys alone are not a unique key — be + // conservative and claim none. + let has_count_values = + aggs.iter().any(|a| matches!(a, AggIntent::CountValues { .. })); + let unique_keys = if by.is_empty() || has_count_values { Vec::new() } else { vec![(0..by.len()).collect()] diff --git a/crates/l2/src/lower.rs b/crates/l2/src/lower.rs index 704d9bd5..ba615992 100644 --- a/crates/l2/src/lower.rs +++ b/crates/l2/src/lower.rs @@ -601,6 +601,10 @@ fn agg_func_to_intent(func: &AggFunc, acc: &AccuracyTarget, col: Option AggIntent::AbsentOverTime, AggFunc::PresentOverTime => AggIntent::PresentOverTime, AggFunc::TimeFn(f) => AggIntent::TimeFn(*f), + AggFunc::Group => AggIntent::Group, + AggFunc::CountValues { label } => AggIntent::CountValues { + label: label.clone(), + }, } } diff --git a/crates/l2/src/relational.rs b/crates/l2/src/relational.rs index 4aac7d62..769c987b 100644 --- a/crates/l2/src/relational.rs +++ b/crates/l2/src/relational.rs @@ -159,6 +159,12 @@ pub enum AggFunc { /// PromQL time / calendar accessor (`timestamp`/`hour`/`day_of_week`/…) → /// `AggIntent::TimeFn` (issue #46). TimeFn(TimeFunc), + /// PromQL `group(v)` → `AggIntent::Group` (issue #49). + Group, + /// PromQL `count_values("l", v)` → `AggIntent::CountValues` (issue #49). + CountValues { + label: String, + }, } /// The Layer-2 relational query IR. From f5fde8be3470c65c213c554337dd9c3a14279072 Mon Sep 17 00:00:00 2001 From: zz_y Date: Fri, 3 Jul 2026 13:04:56 -0600 Subject: [PATCH 2/2] fix(ir): dedupe count_values label that collides with a group-by key `count_values by (job)("job", v)` names the synthesized value-label the same as a group-by key, which emitted a duplicate `job` column in the output schema. PromQL's synthesized label takes precedence, so emit a single column: skip the synthesized label when a column of that name already exists. Found in self-review of #49. Co-Authored-By: Claude Opus 4.8 --- crates/frontend-promql/tests/promql_conformance.rs | 12 ++++++++++++ crates/ir/src/intent_algebra/query_expr.rs | 9 +++++++-- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/crates/frontend-promql/tests/promql_conformance.rs b/crates/frontend-promql/tests/promql_conformance.rs index 169849e6..12d43214 100644 --- a/crates/frontend-promql/tests/promql_conformance.rs +++ b/crates/frontend-promql/tests/promql_conformance.rs @@ -1348,6 +1348,18 @@ fn count_values_accepts_a_parenthesised_label_and_by_grouping() { assert!(sch.columns.iter().any(|c| c.name == "v")); } +#[test] +fn count_values_label_colliding_with_a_group_key_is_not_duplicated() { + // `count_values by (job)("job", v)` — the synthesized label name collides + // with a group-by key. PromQL's synthesized label takes precedence; the + // output must carry a single `job` column, never two. + let qe = ok(r#"count_values by (job) ("job", version)"#); + let sch = qe.output_schema().unwrap(); + let jobs = sch.columns.iter().filter(|c| c.name == "job").count(); + assert_eq!(jobs, 1, "collision deduped, got {:?}", sch.columns); + assert!(sch.columns.iter().any(|c| c.name == "count")); +} + #[test] fn limitk_and_limit_ratio_are_rejected__GAP() { // `limitk`/`limit_ratio` are series-*sampling* operators — they return a diff --git a/crates/ir/src/intent_algebra/query_expr.rs b/crates/ir/src/intent_algebra/query_expr.rs index d12b3765..72581764 100644 --- a/crates/ir/src/intent_algebra/query_expr.rs +++ b/crates/ir/src/intent_algebra/query_expr.rs @@ -498,9 +498,14 @@ impl QueryExpr { // `count_values("l", v)` emits TWO columns: the synthesized // `Utf8` label `l` (the stringified sample value it groups // by) and the per-value count. `output_names[i]` still - // overrides the count column's name if set. + // overrides the count column's name if set. If `l` collides + // with a group-by key of the same name, PromQL's synthesized + // label takes precedence — emit a single column, never a + // duplicate. if let AggIntent::CountValues { label } = intent { - out_cols.push(Column::new(label.clone(), DataType::Utf8, false)); + if !out_cols.iter().any(|c| c.name == *label) { + out_cols.push(Column::new(label.clone(), DataType::Utf8, false)); + } let mut cnt = intent.output_column(&probe); if let Some(name) = output_names.get(i).filter(|s| !s.is_empty()) { cnt.name = name.clone();