[CALCITE-5523] RelToSql converter generates GROUP BY clause with wind…#3431
[CALCITE-5523] RelToSql converter generates GROUP BY clause with wind…#3431JiajunBernoulli wants to merge 1 commit intoapache:mainfrom
Conversation
| return true; | ||
| } | ||
| if (!dialect.supportsNestedAggregations() | ||
| && agg.getInput() instanceof Project |
There was a problem hiding this comment.
What if input is Window object?
There was a problem hiding this comment.
And also agg.getInput() could be a Calc as well.
IMO something is deeply wrong with needNewSubQuery in general in terms of that it uses instanceof Project in multiple places, but does not really care about having Calc or Window on the same position, instead.
One of the possible solutions that could make whole code of RelToSqlConverter simpler, is to do some "normalization" of relational tree inside SqlImplementor::visitRoot(...) method in order to transform it to a form having less possible amount of node types.
But I guess that is something that needs to be discussed on the mailing list.
WDYT?
There was a problem hiding this comment.
When will the input of Aggreagte be Window?
Window only appear when you use ProjectToWindowRule and I think the window will not be input of Aggregate.
There was a problem hiding this comment.
It looks that you are right, at least I cannot reproduce an issue with Window under Aggregate that would also have a problem of aggregation by window expression.
But I guess that part of my comment about Calc is still relevant.
|
Kudos, SonarCloud Quality Gate passed! |
|
It looks like the title and commit's description is not clear. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions. |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions. |
3 participants
…ow expression