LeonidChistov commented on code in PR #3431:
URL: https://github.com/apache/calcite/pull/3431#discussion_r1328128794
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1878,6 +1878,11 @@ && hasSortByOrdinal()) {
&& (hasNestedAgg || hasNestedWindowedAgg)) {
return true;
}
+ if (!dialect.supportsNestedAggregations()
+ && agg.getInput() instanceof Project
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
