-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[CALCITE-7647] Support SELECT * in GROUP BY ALL and ORDER BY ALL #5089
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4301,4 +4301,19 @@ GROUP BY GROUPING SETS ((deptno), ()); | |
|
|
||
| !ok | ||
|
|
||
| # [CALCITE-7647] GROUP BY ALL expands SELECT * to every underlying column; | ||
| # the star columns become grouping keys and the aggregate is excluded | ||
| select *, count(*) as c from (values (1, 'a'), (1, 'a'), (2, 'b')) as t(x, y) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to know if this has been verified through specific database testing which @mihaibudiu also metioned in jira.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question; yes, it's verified.
I ran this exact test case in the DuckDB shell (https://shell.duckdb.org): SELECT *, count(*) AS c
FROM (VALUES (1, 'a'), (1, 'a'), (2, 'b')) AS t(x, y)
GROUP BY ALL
ORDER BY ALL;
┌───────┬─────────┬───────┐
│ x │ y │ c │
├───────┼─────────┼───────┤
│ 1 │ a │ 2 │
│ 2 │ b │ 1 │
└───────┴─────────┴───────┘Same result as test: * expands to (x, y), those become the grouping keys, and COUNT(*) is excluded |
||
| group by all | ||
| order by x; | ||
| +---+---+---+ | ||
| | X | Y | C | | ||
| +---+---+---+ | ||
| | 1 | a | 2 | | ||
| | 2 | b | 1 | | ||
| +---+---+---+ | ||
| (2 rows) | ||
|
|
||
| !ok | ||
|
|
||
| # End agg.iq | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this differ from expand star?
Can this be avoided by doing the rewriteOrderByAll after the star expansion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expandStarForAllRewritejust runs the normal star expansion on the*. I don't call the publicexpandStarbecause it expands the wholeSELECTlist beforeGROUP BY ALLis replaced and that makes the real columns look "not grouped."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you give an example? I don't quite understand either.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's say I had a query like
SELECT *, UPPER(ename) FROM emp GROUP BY ALL. When we first type-check a column, we figure out which columns we're grouping by and cache that answer for good. So if I use the publicexpandStar, it type-checksUPPER(name)before I've replacedGROUP BY ALLwith the actual columns. So internally, that stores "we don't have to group by anything." After that, I can't change the that memory. We get the error "ExpressionEMP.EMPNOis not being grouped."It reports
EMPNO(first column), notUPPER(ename).Note:
SELECT * FROM emp GROUP BY ALLworks fine, but the extraUPPER(ename)makes Calcite type-check early.This helper only touches the
*so nothing gets locked in before we've decided the grouping columns.