Repository navigation
Fix grouping, alias, expression, and modifier lowering - #69
Merged
Merged
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09005e08ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+7673
to
+7675
| let dimensions = extract_dimension_columns_from_select(&sql); | ||
| let explicit_grouping = if dimensions.is_empty() { | ||
| "()".to_string() |
There was a problem hiding this comment.
For queries with a projection star, extract_dimension_columns_from_select() explicitly excludes is_star items, so this replacement omits the columns that DuckDB would normally expand and group. For example, SELECT * EXCLUDE (revenue), AGGREGATE(revenue) AT (ALL) FROM sales_v GROUP BY ALL now becomes GROUP BY () when the star supplies all dimensions, causing a binder error for the projected columns instead of returning each dimension group with the grand total. Expand projection stars before collecting the explicit grouping dimensions.
Useful? React with 👍 / 👎.
nicosuave
force-pushed
the
measure-lowering-fixes
branch
from
November 10, 2026 18:53
334cb1f to
6bca733
Compare
Lowering fixes for reported queries: - GROUP BY ALL with AGGREGATE in HAVING (#55) - ROLLUP, CUBE, and GROUPING SETS subtotals (#56) - expressions over measure aliases in SELECT and HAVING (#57) - CAST and `::` casts, slices, and other expression dimensions, alone or next to plain columns (#58, #60) - set operations whose operands group differently (#59) - HAVING with a joined measure view (#63) - star measure views grouped by unprojected dimensions (#64) Modifier semantics now follow Measures in SQL section 3.5 (#54, #61): - modifiers in one AT (...) apply in written order, and chained AT clauses compose from the outside inward, so `m AT (A B)` equals `m AT (B) AT (A)` - WHERE can be followed by another modifier - AT (VISIBLE SET dim = expr) keeps independent visible filters while replacing the restriction on dim; inseparable predicates are rejected - plain AT (SET ...) warns when it drops a WHERE filter on an ungrouped dimension, as AT (ALL ...) already does This changes results for queries that chain or combine modifiers. The affected expectations in the existing suites are updated. A grouped AGGREGATE() read through a CTE that filters, groups, or limits its rows is rejected, because the grouped form recomputes from the defining view and would silently include the removed rows. Window measures honor the CTE.
An unqualified measure that the queried relation did not define was matched by name against every measure view in the session. A plain view or derived table over a measure view therefore recomputed from whichever view owned that name, ignoring the relation's own filter, and could pick up an unrelated view that happened to share the measure's name. A grouped AGGREGATE() now resolves a measure through the queried view, a CTE that passes it through, or another view or CTE in the same FROM clause, in FROM order. Anything else is rejected with an error that points to the defining view or a CTE.
…ntend DuckDB 1.5.5 has no grammar extension, so the declaration inside the native guard left the FFI registration without a symbol and the build failed. Declare it with the other FFI entry points and stub it when the native frontend is absent, where no relation is traced.
The rewrite cannot tell a plain view from a relation that does not exist. Lower an untraceable measure to error(...) instead of failing the rewrite, so a missing relation still gets DuckDB's own binder error and an existing view or derived table reports why the measure cannot be read through it.
DuckDB 1.5.5 has no grammar extension, so queries there take the text-based path. The fixes in this branch relied on rewrites that were compiled only with the native frontend, and the 1.5.5 suite regressed: CURRENT references and joined measure views stopped working, and the new behavior was absent. - Compile the visible-filter, context-predicate, and grouping-set rewrites for every build. They operate on ordinary SQL expressions, so they reach the AST through accessors that cover both expression APIs. Only the schema probe and the catalog stability check still need a bind context. - Find CURRENT references with a text scanner when the native grammar is unavailable, including the CURRENT(dim) form. - Carry grouping markers for ROLLUP, CUBE, and GROUPING SETS queries without native scopes, and unwrap any marker the rewrite could not consume. - Keep expressions over an aggregate's alias out of the implicit grouping dimensions on the text path. Tests that observe the caller's session (sequence state under repeated volatile calls) move to a native-only file, because the text path executes on a private connection. The trailing comma after a CTE list is DuckDB 2.0 syntax, so the shared test uses the standard form and the reported form is covered natively.
The join-visibility rewrite tested each base row against the query's inner joins, so a row without a match dropped out of the measure. With columns listed in the view, `AGGREGATE(m)` across an inner join counted only joined rows. The fan-out suite defines the opposite: a measure evaluates against its defining view's rows and a join neither multiplies nor removes them. That suite did not catch the change because its views use `SELECT *`, which had no join visibility at all. Inside the visibility check, preserve every row of the measure's view through the joins (inner and cross joins become outer joins on the view's side). A join then never restricts the measure by itself, while a WHERE predicate on a joined column still applies: an unmatched row fails it. Give `SELECT *` measure views the same check by comparing whole rows, since their column list is not recorded. A filter on a joined column previously failed there with a binder error. VISIBLE SET across a join needs listed columns and says so.
Two shapes still differed on DuckDB 1.5.5: - `GROUP BY a` where `a` is the SELECT alias of an expression dimension dropped that dimension from the measure's context. Resolve the name to its expression unless the measure view has a column of that name, matching DuckDB's precedence. - The alias and grouping-set pass takes one SELECT, so CREATE TABLE AS and INSERT kept alias arithmetic unresolved and ROLLUP subtotals at zero. Run the pass on the statement's query body.
Replacing GROUP BY ALL with an explicit list left out the columns a projection star supplies, so `SELECT * EXCLUDE (revenue), AGGREGATE(revenue) AT (ALL) FROM v GROUP BY ALL` failed to bind where it returned rows before. With a bind context, the parser bridge now reports one star item per column of the star, and the querying SELECT uses them as grouping dimensions. That also puts the star's columns into the measure's context, so a plain AGGREGATE() next to a star returns per-group values instead of the grand total. A star routes the statement to bind time for that reason. View definitions keep their star opaque. Without a bind context the columns are unknown, and GROUP BY ALL is left in place as before.
nicosuave
force-pushed
the
measure-lowering-fixes
branch
from
November 10, 2026 19:02
6bca733 to
a5cd2ac
Compare
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.
Fixes to how
AGGREGATE()queries are lowered, on DuckDB 2.0 and 1.5.5. It changes modifier semantics, which alters results for existing queries. Bottom of the stack now that #68 is merged; #70 builds on it.Behavior changes
Modifier composition now follows section 3.5 of Measures in SQL, where
m AT (A B)equalsm AT (B) AT (A):AT (...)apply in written order.ATclauses compose from the outside inward.WHEREreplaces the context;VISIBLEadds the query's filters.On the README sample data this changes, for example:
AT (VISIBLE WHERE region='East')withWHERE year=2023Eastpredicate was ignored)AT (VISIBLE) AT (WHERE region='East')withWHERE year=2023AT (SET year = year - 1) AT (ALL year), year 2024Ten expectations in the existing suites change: eight for modifier order, and two
ROLLUPgrand totals that wereNULL(#56).Three further changes turn silent results into explicit signals:
AT (SET ...)now warns when it drops aWHEREfilter on an ungrouped dimension, asAT (ALL ...)already does.AGGREGATE()resolves a measure only through a relation it can trace: the defining view, a CTE that passes it through, or another view or CTE in the sameFROM. A plain view or derived table over a measure view is rejected. Before, an unqualified measure was matched by name against every measure view in the session, so such a query recomputed from unfiltered rows and could pick up an unrelated view that shared the measure's name.AGGREGATE(), which recomputes from the defining view. Window measures honor the CTE's rows.Both are documented in
LIMITATIONS.md.Joins keep the behavior the fan-out suite defines: a join neither multiplies nor removes a measure's rows. New here is that a
WHEREpredicate on a joined column applies to the rows it matches, for views with listed columns and forSELECT *views.Fixes
AT (VISIBLE SET year = year - 1)ignored the query'sWHERE, so there was no way to shift a dimension and keep a filter. It now keeps independent visible filters while replacing the restriction onyear. Predicates that cannot be separated, such asyear = 2024 OR region = 'North', are rejected. WithoutVISIBLE,AT (SET ...)still evaluates in the grouping context by design, and now warns when that drops a filter.GROUP BY ALLwithAGGREGATE()inHAVING. A projection star next toGROUP BY ALLkeeps its columns in the grouping: on DuckDB 2.0 they are listed from the FROM schema and also partition the measure; on 1.5.5GROUP BY ALLis left in place.ROLLUP,CUBE, andGROUPING SETSsubtotals, including realNULLgroup values.SELECTandHAVING.CAST,::casts, slices, and other expression dimensions. This also fixes a case that predates the reports: an expression dimension next to a plain column was dropped from the measure's context, soSELECT upper(region), year, AGGREGATE(revenue)returned year totals on every row.WHEREfollowed by another modifier.HAVINGandWHEREover a joined measure view, including a filter that compares a view column with a joined column.DuckDB 1.5.5
DuckDB 1.5.5 has no grammar extension, so it uses the text-based path. The predicate, grouping-set, and visible-filter rewrites now compile for both expression APIs and run there too,
CURRENTreferences are found by a text scanner, andROLLUPqueries carry the grouping markers. All fixes above work on 1.5.5, with two notes:Verification
main.Not reproduced: the comment on #56 about
GROUPING function is not supported hereand the comment on #63 aboutHAVINGwithout a join. Neither has a reproduction, and the variants I tried pass.Fixes #54
Fixes #55
Fixes #56
Fixes #57
Fixes #58
Fixes #59
Fixes #60
Fixes #61
Fixes #63
Fixes #64