bchong95 commented on code in PR #4061:
URL: https://github.com/apache/calcite/pull/4061#discussion_r1854900953
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -6513,12 +6552,11 @@ LIMIT 5]]>
LogicalSort(sort0=[$2], dir0=[ASC], fetch=[5])
LogicalAggregate(group=[{0, 1, 2, 3}])
LogicalProject(EMPNO=[$0], ENAME=[$1], DEPTNO=[$2], RANK_VAL=[$3])
- LogicalFilter(condition=[$5])
- LogicalProject(EMPNO=[$0], ENAME=[$1], DEPTNO=[$2], RANK_VAL=[$3],
EXPR$0=[$4], QualifyExpression=[=($3, $4)])
+ LogicalFilter(condition=[$4])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], DEPTNO=[$7], RANK_VAL=[RANK()
OVER (PARTITION BY $1 ORDER BY $7 DESC)], QualifyExpression=[=(RANK() OVER
(PARTITION BY $1 ORDER BY $7 DESC), $9)])
Review Comment:
I think this is a performance regression, since we are computing the window
function multiple times? I believe this is the scenario that the comment "This
is a very specific application of Common Subexpression Elimination" was trying
to handle.
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -3475,53 +3464,52 @@ private static JoinRelType convertJoinType(JoinType
joinType) {
* @param bb Scope within which to resolve identifiers
* @param select Query
* @param orderExprList Additional expressions needed to implement ORDER BY
+ * @param extraList Additional expressions needed to implement QUALIFY
*/
protected void convertAgg(Blackboard bb, SqlSelect select,
- List<SqlNode> orderExprList) {
+ List<SqlNode> orderExprList, List<SqlNode> extraList) {
Review Comment:
nit: maybe we call these `orderByExpressions` and `quailfyExpressions`?
##########
core/src/test/resources/org/apache/calcite/test/SqlToRelConverterTest.xml:
##########
@@ -6568,11 +6606,10 @@ QUALIFY rank_val = (SELECT COUNT(*) FROM emp)]]>
<Resource name="plan">
<![CDATA[
LogicalProject(EMPNO=[$0], ENAME=[$1], DEPTNO=[$2], RANK_VAL=[$3])
- LogicalFilter(condition=[$5])
- LogicalProject(EMPNO=[$0], ENAME=[$1], DEPTNO=[$2], RANK_VAL=[$3],
EXPR$0=[$4], QualifyExpression=[=($3, $4)])
+ LogicalFilter(condition=[$4])
+ LogicalProject(EMPNO=[$0], ENAME=[$1], DEPTNO=[$7], RANK_VAL=[RANK() OVER
(PARTITION BY $1 ORDER BY $7 DESC)], QualifyExpression=[=(RANK() OVER
(PARTITION BY $1 ORDER BY $7 DESC), $9)])
Review Comment:
I think the JIRA mentioned something about the ProjectMerge logic in
RelBuilder not handling the indexes in the window function. Did that ever get
resolved?
##########
core/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java:
##########
@@ -4696,19 +4693,49 @@ private RelNode convertMultisets(final List<SqlNode>
operands,
return ret;
}
- private void convertSelectList(
+ /** Converts a select-list for an aggregate or non-aggregate query,
+ * adding a filter for a QUALIFY clause if present. */
+ private void convertSelectList(Blackboard bb, Blackboard measureBb,
+ SqlSelect select, List<SqlNode> orderExprList, List<SqlNode> extraList,
+ @Nullable SqlNode qualify) {
+ if (qualify != null) {
+ // Add the QUALIFY expression to the select-list,
+ // convert the extended list,
+ // filter by the last field,
+ // and remove the last field.
+ convertSelectList(bb, measureBb, select, orderExprList,
+ ImmutableList.of(addAlias(qualify, "QualifyExpression")), null);
+ bb.setRoot(
+ relBuilder.push(bb.root())
+ .filter(last(relBuilder.fields()))// filter on last column
Review Comment:
That's a neat way of doing this :)
--
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]
