[calcite] branch master updated: [CALCITE-3961] VolcanoPlanner.prunedNodes info is lost when duplicate relNode is discarded (Botong Huang)

[hy...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

hyuan pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/calcite.git


The following commit(s) were added to refs/heads/master by this push:
     new 2a4779f  [CALCITE-3961] VolcanoPlanner.prunedNodes info is lost when duplicate relNode is discarded (Botong Huang)
2a4779f is described below

commit 2a4779f478fea75c1a7b075b8da50b20b6fda9bb
Author: botong.huang <bo...@alibaba-inc.com>
AuthorDate: Sun Apr 26 17:30:58 2020 -0700

    [CALCITE-3961] VolcanoPlanner.prunedNodes info is lost when duplicate relNode is discarded (Botong Huang)
    
    Close #1949
---
 .../org/apache/calcite/plan/volcano/RelSet.java    | 24 ++++++++--------------
 .../calcite/plan/volcano/VolcanoPlanner.java       | 18 +++++++++++++++-
 .../calcite/plan/volcano/VolcanoRuleCall.java      |  5 -----
 3 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java b/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
index 6f5905c..22cb936 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/RelSet.java
@@ -26,6 +26,7 @@ import org.apache.calcite.plan.RelTraitSet;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.convert.Converter;
 import org.apache.calcite.rel.core.CorrelationId;
+import org.apache.calcite.rel.core.Spool;
 import org.apache.calcite.rel.metadata.RelMetadataQuery;
 import org.apache.calcite.util.trace.CalciteTrace;
 
@@ -361,26 +362,19 @@ class RelSet {
 
     Set<RelNode> parentRels = new HashSet<>(parents);
     for (RelNode otherRel : otherSet.rels) {
-      if (planner.prunedNodes.contains(otherRel)) {
-        continue;
-      }
-
-      boolean pruned = false;
-      if (parentRels.contains(otherRel)) {
-        // if otherRel is a enforcing operator e.g.
-        // Sort, Exchange, do not prune it.
+      if (!(otherRel instanceof Spool)
+          && !otherRel.isEnforcer()
+          && parentRels.contains(otherRel)) {
+        // If otherRel is a enforcing operator e.g.
+        // Sort, Exchange, do not prune it. Just in
+        // case it is not marked as an enforcer.
         if (otherRel.getInputs().size() != 1
             || otherRel.getInput(0).getTraitSet()
                 .satisfies(otherRel.getTraitSet())) {
-          pruned = true;
+          planner.prune(otherRel);
         }
       }
-
-      if (pruned) {
-        planner.prune(otherRel);
-      } else {
-        planner.reregister(this, otherRel);
-      }
+      planner.reregister(this, otherRel);
     }
 
     // Has another set merged with this?
diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
index 1dd391e..89a5b4e 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java
@@ -881,6 +881,7 @@ public class VolcanoPlanner extends AbstractRelOptPlanner {
             rel.getId(), equivRel.getId());
 
         mapDigestToRel.put(key, equivRel);
+        checkPruned(equivRel, rel);
 
         RelSubset equivRelSubset = getSubset(equivRel);
 
@@ -940,11 +941,24 @@ public class VolcanoPlanner extends AbstractRelOptPlanner {
       assert equivRel.getClass() == rel.getClass();
       assert equivRel.getTraitSet().equals(rel.getTraitSet());
 
+      checkPruned(equivRel, rel);
       return;
     }
 
     // Add the relational expression into the correct set and subset.
-    addRelToSet(rel, set);
+    if (!prunedNodes.contains(rel)) {
+      addRelToSet(rel, set);
+    }
+  }
+
+  /**
+   * Prune rel node if the latter one (identical with rel node)
+   * is already pruned.
+   */
+  private void checkPruned(RelNode rel, RelNode duplicateRel) {
+    if (prunedNodes.contains(duplicateRel)) {
+      prunedNodes.add(rel);
+    }
   }
 
   /**
@@ -1144,6 +1158,8 @@ public class VolcanoPlanner extends AbstractRelOptPlanner {
           "left", equivExp.getRowType(),
           "right", rel.getRowType(),
           Litmus.THROW);
+      checkPruned(equivExp, rel);
+
       RelSet equivSet = getSet(equivExp);
       if (equivSet != null) {
         LOGGER.trace(
diff --git a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
index 990a48c..2ac4f0e 100644
--- a/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
+++ b/core/src/main/java/org/apache/calcite/plan/volcano/VolcanoRuleCall.java
@@ -124,11 +124,6 @@ public class VolcanoRuleCall extends RelOptRuleCall {
         volcanoPlanner.getListener().ruleProductionSucceeded(event);
       }
 
-      final RelNode relCopy = rel;
-      if (rels[0].getInputs().stream().anyMatch(n -> n == relCopy)) {
-        volcanoPlanner.prune(rels[0]);
-      }
-
       if (this.getRule() instanceof SubstitutionRule
           && ((SubstitutionRule) getRule()).autoPruneOld()) {
         volcanoPlanner.prune(rels[0]);