Repository navigation
Conversation
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
This could benefit from some quidem tests too.
| resultSelector)); | ||
| final Method groupByMultipleMethod; | ||
| if (hasEmptyGroup) { | ||
| // The key of the empty grouping set is a constant: each grouping |
There was a problem hiding this comment.
it would be much better to explain what happens here without references to calcite issues closed long ago.
There was a problem hiding this comment.
Done, I had added comments to explain the behavior on its own: the key of the empty grouping set is a constant, built the same way as the key the selector for that grouping set computes for any input row, and passed to groupByMultiple so that the empty grouping set produces its single row when the input is empty, just as a query with no GROUP BY produces a row over an empty input.
| builder.append("emptyGroupKey", | ||
| keyPhysType.record(keyExpressions)); | ||
| arguments.add(emptyGroupKey); | ||
| groupByMultipleMethod = BuiltInMethod.GROUP_BY_MULTIPLE2.method; |
There was a problem hiding this comment.
it would help a lot the reviewers if you showed in a comment the generated Java code.
| } | ||
|
|
||
| public void end(Sink sink) throws InterruptedException { | ||
| if (accumulators.isEmpty() && grouping.isEmpty()) { |
There was a problem hiding this comment.
I think the interpreter is hopelessly unmaintained, but it's fine to fix bugs here and there.
There was a problem hiding this comment.
Agreed and I wouldn't want to invest much in the interpreter either.
The fix is only a few lines in AggregateNode.Grouping.end, and it keeps the two runtimes consistent now that PruneEmptyRules no longer prunes an aggregate with an empty grouping set: without it, the interpreter would still return 0 rows while the enumerable path returns the grand-total row. I've also dropped the stale issue reference from the comment there.
Thanks for the suggestion! @mihaibudiu I had added related quidem and validated in postgresql in https://onecompiler.com/postgresql/456r8bnfy |
what is the result in oracle/sql server? |
|
Do you really expect the results to differ? |
I tested it on SQL Server, and it doesn't support this syntax. I believe PostgreSQL is a more convincing solution in this case. |
|
oracle: |
|
This is rather strange if you ask me. This makes the semantics of this query not really compositional as the union of the implied component queries. |
|
As concluded in the Jira discussion, the current PR implementation aligns with that outcome; barring any further objections, I will proceed to merge it. cc @mihaibudiu @iwanttobepowerful |
…ws on empty input
ac3c7ce to
4f399f9
Compare
|
xuzifu666
added
the
LGTM-will-merge-soon
jira: https://issues.apache.org/jira/browse/CALCITE-7828