[calcite] branch master updated: [CALCITE-3981] Volcano.register should not return stale subset (Botong Huang)

[hyuan]

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 df5f447  [CALCITE-3981] Volcano.register should not return stale 
subset (Botong Huang)
df5f447 is described below

commit df5f4470e4257e8e7057664d4af3af3f37b6559b
Author: botong.huang <botong.hu...@alibaba-inc.com>
AuthorDate: Sat Apr 18 22:15:30 2020 -0700

    [CALCITE-3981] Volcano.register should not return stale subset (Botong 
Huang)
    
    When a subset is registered, registerImpl() and registerSubset() currently
    simply returns the subset itself. The problem is that subset can become 
stale
    when relSets get merged (for example in ensureRegistered() and 
registerSubset()
    "merge(set, subset.set)"). As a result, a stale subset might be returned 
from
    registerImpl, and the newly registering subtree might get registered
    recursively on top of the stale subset (see AbstractRelNode.onRegister()). 
This
    is a leak because once a relSet is merged into others and becomes stale, it
    should not be used to connect new relNodes.
    
    With CALCITE-3755, subsets can now be directly matched by rules. This opens
    another source of stale subset leak: (1) An active subset gets matched, the
    RuleMatch gets queued in RuleQueue. (2) The subset becomes stale due to 
relSet
    merge. (3) The rule match in (1) is popped from queue and fired. (4) In 
OnMatch
    the rule gets the stale subset, builds new rels on top of it and regsiter 
the
    new rels. In this case, the entire new rel subtree will be registered on 
top of
    the stale subset as is.
    
    Close #1966
---
 .../main/java/org/apache/calcite/plan/volcano/VolcanoPlanner.java  | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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 90030d0..d7dcd5f 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
@@ -628,6 +628,7 @@ public class VolcanoPlanner extends AbstractRelOptPlanner {
           "equivRel rowtype",
           equivRel.getRowType(),
           Litmus.THROW);
+      equivRel = ensureRegistered(equivRel, null);
       set = getSet(equivRel);
     }
     return registerImpl(rel, set);
@@ -643,7 +644,7 @@ public class VolcanoPlanner extends AbstractRelOptPlanner {
           merge(equivSubset.set, subset.set);
         }
       }
-      result = subset;
+      result = canonize(subset);
     } else {
       result = register(rel, equivRel);
     }
@@ -1024,7 +1025,7 @@ public class VolcanoPlanner extends AbstractRelOptPlanner 
{
       set = set.equivalentSet;
     } while (set.equivalentSet != null);
     return set.getOrCreateSubset(
-        subset.getCluster(), subset.getTraitSet());
+        subset.getCluster(), subset.getTraitSet(), subset.isRequired());
   }
 
   /**
@@ -1331,7 +1332,7 @@ public class VolcanoPlanner extends AbstractRelOptPlanner 
{
       LOGGER.trace("Register #{} {}, and merge sets", subset.getId(), subset);
       merge(set, subset.set);
     }
-    return subset;
+    return canonize(subset);
   }
 
   // implement RelOptPlanner