[CALCITE-4003] Disallow cross convention matching and generation in T…

…ransformationRule
apache · Jun 10, 2020 · 0c8d0fe · 0c8d0fe
1 parent f9e8413
commit 0c8d0fe
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 2 deletions.
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
@@ -21,8 +21,10 @@
 import org.apache.calcite.plan.RelOptRuleCall;
 import org.apache.calcite.plan.RelOptRuleOperand;
 import org.apache.calcite.plan.RelOptRuleOperandChildPolicy;
+import org.apache.calcite.rel.PhysicalNode;
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.rel.rules.SubstitutionRule;
+import org.apache.calcite.rel.rules.TransformationRule;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -92,6 +94,12 @@ protected VolcanoRuleCall(
   // implement RelOptRuleCall
   public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
       RelHintsPropagator handler) {
+    if (rel instanceof PhysicalNode
+        && rule instanceof TransformationRule) {
+      throw new RuntimeException(
+          rel + " is a PhysicalNode, which is not allowed in " + rule);
+    }
+
     rel = handler.propagate(rels[0], rel);
     if (LOGGER.isDebugEnabled()) {
       LOGGER.debug("Transform to: rel#{} via {}{}", rel.getId(), getRule(),
@@ -136,7 +144,9 @@ public void transformTo(RelNode rel, Map<RelNode, RelNode> equiv,
         volcanoPlanner.ensureRegistered(
             entry.getKey(), entry.getValue());
       }
-      volcanoPlanner.ensureRegistered(rel, rels[0]);
+      // The subset is not used, but we need it, just for debugging
+      //noinspection unused
+      RelSubset subset = volcanoPlanner.ensureRegistered(rel, rels[0]);
       rels[0].getCluster().invalidateMetadataQuery();
 
       if (volcanoPlanner.getListener() != null) {
@@ -349,6 +359,10 @@ private void matchRecurse(int solve) {
       }
 
       for (RelNode rel : successors) {
+        if (operand.getRule() instanceof TransformationRule
+            && rel.getConvention() != previous.getConvention()) {
+          continue;
+        }
         if (!operand.matches(rel)) {
           continue;
         }

diff --git a/core/src/main/java/org/apache/calcite/rel/rules/ProjectRemoveRule.java b/core/src/main/java/org/apache/calcite/rel/rules/ProjectRemoveRule.java
@@ -66,6 +66,7 @@ public void onMatch(RelOptRuleCall call) {
           childProject.getInput(), childProject.getProjects(),
           project.getRowType());
     }
+    stripped = convert(stripped, project.getConvention());
     call.transformTo(stripped);
   }
 

diff --git a/core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRule.java b/core/src/main/java/org/apache/calcite/rel/rules/SortRemoveRule.java
@@ -61,7 +61,8 @@ public SortRemoveRule(RelBuilderFactory relBuilderFactory) {
     final RelCollation collation = sort.getCollation();
     assert collation == sort.getTraitSet()
         .getTrait(RelCollationTraitDef.INSTANCE);
-    final RelTraitSet traits = sort.getInput().getTraitSet().replace(collation);
+    final RelTraitSet traits = sort.getInput().getTraitSet()
+        .replace(collation).replace(sort.getConvention());
     call.transformTo(convert(sort.getInput(), traits));
   }
 }
diff --git a/site/_docs/history.md b/site/_docs/history.md
@@ -34,6 +34,8 @@ Downloads are available on the
 
 * [<a href="https://issues.apache.org/jira/browse/CALCITE-4032">CALCITE-4032</a>]
   Mark `CalcMergeRule` as `TransformationRule`
+* [<a href="https://issues.apache.org/jira/browse/CALCITE-4003">CALCITE-4003</a>]
+  Disallow cross convention matching and `PhysicalNode` generation in `TransformationRule`
 
 ## <a href="https://github.com/apache/calcite/releases/tag/calcite-1.23.0">1.23.0</a> / 2020-05-23
 {: #v1-23-0}