[CALCITE-7594] Support GROUP BY ALL#5009
Conversation
|
CI failure is |
|
Where do you see that error? |
|
|
I see this error, which also tells you how to fix it: |
|
Thanks for replying, I completely missed that. Updated my PR |
tisyabhatia
force-pushed
the
CALCITE-7594
branch
from
6145111 to
dd04ebb
Compare
|
See the comment I left on #5010 |
|
In addition, there is a conformance flag called isGroupByAlias which should be tested in combination with this. |
|
There is a pr to trace this jira, should this one can be closed? |
|
Yes @xuzifu666, here it is: https://issues.apache.org/jira/browse/CALCITE-7594 |
When GROUP BY ALL appears with no grouping items, group by every expression in the SELECT clause that does not contain an aggregate or window function and is not a measure. Parser forks the ALL branch on LOOKAHEAD(2); a trailing grouping list keeps the existing CALCITE-5089 ALL/DISTINCT set-quantifier behaviour; nothing trailing emits a GROUP_BY_ALL marker that the validator expands before base group validation. Constants are kept as grouping keys (grouping only by constants over empty input returns 0 rows, matching standard SQL and major dialects); SELECT * is rejected. Includes parser, validator, and execution tests plus reference docs.
tisyabhatia
force-pushed
the
CALCITE-7594
branch
from
dd04ebb to
55adcaf
Compare
|
|
I believe all review feedback has been addressed and the change is complete with tests. Could a committer take a look when you have a chance to move it toward approval/merge? Happy to make any further adjustments if needed. Thanks! |
|
In general, while you address feedback you should create new commits, which enable the reviewers to see what's changed. The commits will be squashed after the PR is accepted. |
|
|
||
| /** Test case for [CALCITE-7594] GROUP BY ALL: grouping only by a constant | ||
| * over empty input returns 0 rows. */ | ||
| @Test void testGroupByAllOverEmptyInput() { |
There was a problem hiding this comment.
I think that the preferred way to write such tests is to use quidem files (suffix .iq), but this works too.
mihaibudiu
added
the
LGTM-will-merge-soon
|
@mihaibudiu once this PR (and related, CALCITE-7597) is merged, is there any possibility of it going out in a patch release, or would it have to wait for the next rollout? (for context, I'm an intern working on adding support for these features to Flink) |
|
I don't think people will speed up a release for this feature, and 1.42 just happened. But you don't really need a release to incorporate these in another project. In our project we periodically choose a commit from the Calcite main branch and build against that. Especially for testing and benchmarking you should be able to do that. |
3 participants
JIRA: https://issues.apache.org/jira/browse/CALCITE-7594