[GitHub] [calcite] chunweilei commented on a change in pull request #1166: [CALCITE-3004] RexOver is incorrectly pushed down in ProjectSetOpTran…

[GitBox <gi...@apache.org>]

chunweilei commented on a change in pull request #1166: [CALCITE-3004] RexOver is incorrectly pushed down in ProjectSetOpTran…
URL: https://github.com/apache/calcite/pull/1166#discussion_r275847509
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/rel/rules/ProjectSetOpTransposeRule.java
 ##########
 @@ -90,23 +91,35 @@ public void onMatch(RelOptRuleCall call) {
     List<RelNode> newSetOpInputs = new ArrayList<>();
     int[] adjustments = pushProject.getAdjustments();
 
-    // push the projects completely below the setop; this
-    // is different from pushing below a join, where we decompose
-    // to try to keep expensive expressions above the join,
-    // because UNION ALL does not have any filtering effect,
-    // and it is the only operator this rule currently acts on
-    for (RelNode input : setOp.getInputs()) {
-      // be lazy:  produce two ProjectRels, and let another rule
-      // merge them (could probably just clone origProj instead?)
-      Project p = pushProject.createProjectRefsAndExprs(input, true, false);
-      newSetOpInputs.add(pushProject.createNewProject(p, adjustments));
+    final RelNode node;
+    if (RexOver.containsOver(origProj.getProjects(), null)) {
+      // should not push over past setop but can push its operand down.
+      for (RelNode input : setOp.getInputs()) {
+        Project p = pushProject.createProjectRefsAndExprs(input, true, false);
+        // make sure that it is not a trivial project to avoid infinite loop.
+        if (p.getRowType().equals(input.getRowType())) {
+          return;
+        }
+        newSetOpInputs.add(p);
+      }
+      SetOp newSetOp =
+          setOp.copy(setOp.getTraitSet(), newSetOpInputs);
+      node = pushProject.createNewProject(newSetOp, adjustments);
+    } else {
+      // push some expressions below the setop; this
 
 Review comment:
   There is `if..else..` in that I don't want to change the plan of some cases. For instance,  current plan of 
   ```
   select ename,avg(empno) from 
   (select * from emp as e1 union all select * from emp as e2)
   ```
   is
   ```
   LogicalAggregate(group=[{0}], EXPR$1=[AVG($1)])
     LogicalUnion(all=[true])
       LogicalProject(ENAME=[$1], EMPNO=[$0])
         LogicalTableScan(table=[[CATALOG, SALES, EMP]])
       LogicalProject(ENAME=[$1], EMPNO=[$0])
         LogicalTableScan(table=[[CATALOG, SALES, EMP]])
   ```
   But if without` if..else..`, the plan becomes
   ```
   LogicalAggregate(group=[{0}], EXPR$1=[AVG($1)])
     LogicalProject(ENAME=[$1], EMPNO=[$0])
       LogicalUnion(all=[true])
         LogicalProject(EMPNO=[$0], ENAME=[$1])
           LogicalTableScan(table=[[CATALOG, SALES, EMP]])
         LogicalProject(EMPNO=[$0], ENAME=[$1])
           LogicalTableScan(table=[[CATALOG, SALES, EMP]])
   ```
   which seems redundant.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services