feat(sql): materialize expression GROUP BY and reducer arguments (#110) - #119
Merged
Merged
Conversation
`Aggregate.by` holds positional ColumnIds and each reducer holds one input
column, so neither could hold an expression. `GROUP BY date_trunc('minute', ts)`
— the canonical time-series bucketing idiom, and the SQL counterpart of a PromQL
range selector — was rejected, which meant the SQL front end could not express
windowed aggregation at all. `SUM(bytes * 8)` was rejected for the same reason.
Materialize each grouping / reducer expression as a derived column in a Project
beneath the Aggregate, then group and reduce over that column. The projection
also carries through the plain columns the aggregate still references, since a
Project replaces its child's schema rather than extending it.
The projection is inserted only when something actually needs deriving, so every
query that lowers today keeps its exact tree shape. Two expressions with the same
value are materialized once, so `SUM(v*2), MIN(v*2)` share a column.
A Project carries one relation qualifier for all its columns, so `a.k` and `b.k`
cannot both survive it. That is reported as an ambiguity — but only when the
projection is actually inserted, otherwise a join whose keys need no
materialization would regress.
GROUPING SETS / ROLLUP / CUBE are a different problem: several grouping levels
plus a `__grouping_id` discriminator from one scan, which `Aggregate.by` cannot
express. They previously failed as "non-column GROUP BY expression", implying
they were the same issue; they now fail as "multi-level grouping" and are
tracked in #118.
Supersedes the rejection this change replaces: `SUM(<expr>)` and
`median(<expr>)` used to error rather than lower with `col: None` (#115). They
now lower with `col: Some(<derived>)`. The #115 invariant — a SQL reducer never
reaches L3 with `col: None` — still holds and is still asserted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #110 (expression
GROUP BY+ aggregates over expressions). Multi-level grouping is split out to #118.The problem
Aggregate.byholds positionalColumnIds and each reducer holds one input column, so neither can hold an expression:The first is the canonical time-series bucketing idiom — the SQL counterpart of a PromQL range selector. Without it the SQL front end could not express windowed aggregation at all, which is the query family the sketch layer exists to accelerate.
The fix
Materialize each grouping / reducer expression as a derived column in a
Projectbeneath theAggregate, then group and reduce over that column:DataFusion strips
AS mfrom a grouping expression, so the derived column takes the aggregate schema’s field name verbatim — that is the name the enclosing Projection resolves against.The projection also carries through the plain columns the aggregate still references, because a
Projectreplaces its child’s schema rather than extending it.Four properties worth calling out, each pinned by a test:
SUM(v*2), MIN(v*2)bind the same derived column.WHEREstill folds onto theScan. The projection goes above the scan, not between the filter and it.Projectcarries one relation qualifier for all its columns, soa.kandb.kcannot both survive it. Crucially this is only checked when the projection is inserted — an earlier draft checked eagerly and regressedSELECT t.k, u.k, SUM(t.v) … GROUP BY t.k, u.k, which needs no materialization at all.Behaviour change worth reviewing
Three tests asserted that
SUM(<expr>)/median(<expr>)/approx_percentile_cont(<expr>, φ)are rejected. I wrote two of them myself in #115 and #111, on the grounds that an expression argument would otherwise reach L3 ascol: Noneand be silently dropped.That reasoning was about the silent drop, not about the expression. Those queries now lower correctly with
col: Some(<derived>), so the tests are rewritten to assert the binding rather than the rejection. The #115 invariant still holds and is still asserted: a SQL reducer never reaches L3 withcol: None.GROUPING SETS / ROLLUP / CUBE
Not fixed here, and not the same problem. They emit several grouping levels plus a
__grouping_iddiscriminator from one scan;Aggregate.byis a single key set. Previously they failed asnon-column GROUP BY expression: ROLLUP (…), which implied they were the same issue asdate_trunc. They now fail asmulti-level grouping: ROLLUPand are tracked in #118 with two options written up.Verification
cargo fmt --allnot run —maincarries pre-existing rustfmt diffs, so it would bury the change. Both touched files have the same violation count as onmain.🤖 Generated with Claude Code