Skip to content

Commit f9458d6

Browse files
zzylolclaude
andcommitted
feat(core): route tabular GROUP BY through positional Aggregate.by (key in schema)
The single-aggregate fused path wraps GROUP BY keys in a name-based Partition (the PromQL streaming-sketch canonical shape) whose output schema is the child's — so the group key is not an output column. That's correct for time series but wrong for SQL, where `SELECT k, agg(...) GROUP BY k` projects `k`. The fused path is now gated on a *time-series* leaf (`relational::leaf_is_tabular` = the leftmost Source carries a resolved schema, i.e. Source::Table). Tabular single-agg GROUP BY falls through to the positional `Aggregate.by` path, so the key lands in the output schema and the enclosing SELECT projection resolves it. PromQL's fused-Partition shape is unchanged. - relational: factor out `leaf_source`; add `leaf_is_tabular`; `source_name` now delegates to `leaf_source`. Test: `SELECT service, SUM(bytes) GROUP BY service` → Aggregate.by=[1], Sum{col:3}, and the root projection schema is [service:Utf8, sum:Int64]. 111 tests green, clippy -D warnings clean, fmt clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d96e032 commit f9458d6

3 files changed

Lines changed: 51 additions & 12 deletions

File tree

‎crates/core/src/intent_algebra/lower.rs‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,11 +87,15 @@ pub fn convert(
8787
having,
8888
input,
8989
} => {
90-
// Single-statistic aggregate (no HAVING) fuses: a `Window` input
91-
// becomes `Window { Aggregate { by: [] } }`; GROUP BY keys wrap the
92-
// result in a `Partition`. The reducer's input column resolves
93-
// against the aggregate's *direct* input (the scan under any window).
94-
if aggs.len() == 1 && having.is_none() {
90+
// Single-statistic aggregate (no HAVING) over a *time-series* leaf
91+
// fuses: a `Window` input becomes `Window { Aggregate { by: [] } }`;
92+
// GROUP BY keys wrap the result in a `Partition` (the streaming
93+
// sketch canonical shape). Tabular (SQL) GROUP BY instead falls
94+
// through to the positional `Aggregate.by` path below, so the group
95+
// keys land in the output schema (a SELECT projects them). The
96+
// reducer's input column resolves against the aggregate's *direct*
97+
// input (the scan under any window).
98+
if aggs.len() == 1 && having.is_none() && !input.leaf_is_tabular() {
9599
let (agg_input_l2, window): (&LQueryExpr, Option<(_, _)>) = match input.as_ref() {
96100
LQueryExpr::Window {
97101
duration,

‎crates/core/src/intent_algebra/relational.rs‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -223,10 +223,10 @@ impl QueryExpr {
223223
}
224224
}
225225

226-
/// Outermost metric/table name from the first `Source` leaf.
227-
pub fn source_name(&self) -> Option<&str> {
226+
/// The leftmost `Source` leaf.
227+
pub fn leaf_source(&self) -> Option<&SourceSpec> {
228228
match self {
229-
QueryExpr::Source(s) => Some(&s.name),
229+
QueryExpr::Source(s) => Some(s),
230230
QueryExpr::Filter { input, .. }
231231
| QueryExpr::Project { input, .. }
232232
| QueryExpr::Aggregate { input, .. }
@@ -236,13 +236,27 @@ impl QueryExpr {
236236
| QueryExpr::TopK { input, .. }
237237
| QueryExpr::Sort { input, .. }
238238
| QueryExpr::Limit { input, .. }
239-
| QueryExpr::PromQLSubquery { input, .. } => input.source_name(),
240-
QueryExpr::Merge { inputs } => inputs.first()?.source_name(),
239+
| QueryExpr::PromQLSubquery { input, .. } => input.leaf_source(),
240+
QueryExpr::Merge { inputs } => inputs.first()?.leaf_source(),
241241
QueryExpr::Join { left, .. }
242242
| QueryExpr::SetOp { left, .. }
243-
| QueryExpr::BinaryOp { lhs: left, .. } => left.source_name(),
244-
QueryExpr::LetBinding { body, .. } => body.source_name(),
243+
| QueryExpr::BinaryOp { lhs: left, .. } => left.leaf_source(),
244+
QueryExpr::LetBinding { body, .. } => body.leaf_source(),
245245
QueryExpr::Ref(_) => None,
246246
}
247247
}
248+
249+
/// Outermost metric/table name from the first `Source` leaf.
250+
pub fn source_name(&self) -> Option<&str> {
251+
self.leaf_source().map(|s| s.name.as_str())
252+
}
253+
254+
/// Whether the leftmost `Source` leaf carries a resolved schema — i.e. it is
255+
/// a SQL table (`Source::Table`). Time-series (PromQL) leaves return
256+
/// `false`. The converter uses this to keep the time-series fused-Partition
257+
/// canonical shape for PromQL while routing tabular GROUP BY through a
258+
/// positional `Aggregate.by` (so group keys land in the output schema).
259+
pub fn leaf_is_tabular(&self) -> bool {
260+
self.leaf_source().is_some_and(|s| s.schema.is_some())
261+
}
248262
}

‎crates/lower/tests/sql_lowering.rs‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,27 @@ async fn projection_over_aggregate_resolves_output_types_via_output_names() {
134134
);
135135
}
136136

137+
#[tokio::test]
138+
async fn single_agg_group_by_keeps_key_in_output_schema() {
139+
// A tabular single-aggregate GROUP BY routes through the positional
140+
// Aggregate.by path (not the PromQL fused-Partition shape), so the group
141+
// key is a real output column the enclosing SELECT projection resolves.
142+
let qe = lower("SELECT service, SUM(bytes) FROM metrics GROUP BY service").await;
143+
let (by, aggs) = find_aggregate(&qe).expect("expected an Aggregate (not a Partition)");
144+
assert_eq!(by, &vec![1], "GROUP BY service → Aggregate.by column 1");
145+
assert!(matches!(aggs.as_slice(), [AggIntent::Sum { col: Some(3) }]));
146+
147+
// Both the group key and the aggregate resolve in the root projection schema.
148+
let schema = qe.output_schema().expect("root projection schema");
149+
assert_eq!(schema.columns.len(), 2);
150+
assert_eq!(
151+
schema.columns[0].dtype,
152+
DataType::Utf8,
153+
"service is in the output"
154+
);
155+
assert_eq!(schema.columns[1].dtype, DataType::Int64, "SUM(bytes)");
156+
}
157+
137158
#[tokio::test]
138159
async fn count_star_is_count_intent() {
139160
let qe = lower("SELECT COUNT(*) FROM metrics").await;

0 commit comments

Comments
 (0)