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 ff71266 [CALCITE-3927] RelSubset is not fired for rule when set gets
merged (Botong Huang)
ff71266 is described below
commit ff71266352df23208a75b9076045e89f30b9bb0b
Author: botong.huang <[email protected]>
AuthorDate: Wed Apr 15 17:58:02 2020 -0700
[CALCITE-3927] RelSubset is not fired for rule when set gets merged (Botong
Huang)
In VolcanoPlanner, when set gets merged, planner fires rules again for
RelNodes
in both sets, but not for RelSubset. We might miss something because of
this.
If all the logical transformation rules and physical implementation rules
are
separated out in different stage and physical rules don't do logical work,
we
might be OK. But the reality is that all the things are mixed together at
the
moment.
Close #1922
---
.../org/apache/calcite/plan/volcano/RelSet.java | 4 ++
.../apache/calcite/plan/volcano/PlannerTests.java | 15 +++--
.../calcite/plan/volcano/VolcanoPlannerTest.java | 76 ++++++++++++++++++++++
3 files changed, 89 insertions(+), 6 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 789b5b8..fc5d52a 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
@@ -433,5 +433,9 @@ class RelSet {
assert planner.getSet(rel) == this;
planner.fireRules(rel);
}
+ // Fire rule match on subsets as well
+ for (RelSubset subset : subsets) {
+ planner.fireRules(subset);
+ }
}
}
diff --git
a/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
b/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
index ed1c927..e52e079 100644
--- a/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
+++ b/core/src/test/java/org/apache/calcite/plan/volcano/PlannerTests.java
@@ -22,6 +22,7 @@ import org.apache.calcite.plan.RelOptCost;
import org.apache.calcite.plan.RelOptPlanner;
import org.apache.calcite.plan.RelOptRule;
import org.apache.calcite.plan.RelOptRuleCall;
+import org.apache.calcite.plan.RelTrait;
import org.apache.calcite.plan.RelTraitSet;
import org.apache.calcite.rel.AbstractRelNode;
import org.apache.calcite.rel.BiRel;
@@ -60,13 +61,15 @@ class PlannerTests {
static final Convention PHYS_CALLING_CONVENTION_2 =
new Convention.Impl("PHYS_2", RelNode.class) {
- @Override public boolean canConvertConvention(Convention toConvention)
{
- return true;
- }
+ };
- @Override public boolean useAbstractConvertersForConversion(
- RelTraitSet fromTraits, RelTraitSet toTraits) {
- return true;
+ static final Convention PHYS_CALLING_CONVENTION_3 =
+ new Convention.Impl("PHYS_3", RelNode.class) {
+ @Override public boolean satisfies(RelTrait trait) {
+ if (trait.equals(PHYS_CALLING_CONVENTION)) {
+ return true;
+ }
+ return super.satisfies(trait);
}
};
diff --git
a/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
b/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
index 9689acb..d93a223 100644
--- a/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
+++ b/core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
@@ -52,6 +52,7 @@ import static
org.apache.calcite.plan.volcano.PlannerTests.NoneLeafRel;
import static org.apache.calcite.plan.volcano.PlannerTests.NoneSingleRel;
import static
org.apache.calcite.plan.volcano.PlannerTests.PHYS_CALLING_CONVENTION;
import static
org.apache.calcite.plan.volcano.PlannerTests.PHYS_CALLING_CONVENTION_2;
+import static
org.apache.calcite.plan.volcano.PlannerTests.PHYS_CALLING_CONVENTION_3;
import static org.apache.calcite.plan.volcano.PlannerTests.PhysBiRel;
import static org.apache.calcite.plan.volcano.PlannerTests.PhysLeafRel;
import static org.apache.calcite.plan.volcano.PlannerTests.PhysLeafRule;
@@ -255,6 +256,36 @@ class VolcanoPlannerTest {
}
/**
+ * Tests that VolcanoPlanner should fire rule match from subsets after a
+ * RelSet merge. The rules matching for a RelSubset should be able to fire
+ * on the subsets that are merged into the RelSets.
+ */
+ @Test void testSetMergeMatchSubsetRule() {
+ VolcanoPlanner planner = new VolcanoPlanner();
+ planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
+ planner.addRelTraitDef(RelCollationTraitDef.INSTANCE);
+
+ planner.addRule(new PhysLeafRule());
+ planner.addRule(new GoodSingleRule());
+ planner.addRule(new PhysSingleInputSetMergeRule());
+ final List<String> buf = new ArrayList<>();
+ planner.addRule(new PhysSingleSubsetRule(buf));
+
+ RelOptCluster cluster = newCluster(planner);
+ NoneLeafRel leafRel = new NoneLeafRel(cluster, "a");
+ NoneSingleRel singleRel = new NoneSingleRel(cluster, leafRel);
+ RelNode convertedRel = planner
+ .changeTraits(singleRel, cluster.traitSetOf(PHYS_CALLING_CONVENTION));
+ planner.setRoot(convertedRel);
+ RelNode result = planner.chooseDelegate().findBestExp();
+ assertTrue(result instanceof PhysSingleRel);
+ assertThat(sort(buf),
+ equalTo(
+ sort("PhysSingleRel:Subset#0.PHYS.[]",
+ "PhysSingleRel:Subset#0.PHYS_3.[]")));
+ }
+
+ /**
* Tests transformation of a single+leaf from NONE to PHYS. In the past,
* this one didn't work due to the definition of ReformedSingleRule.
*/
@@ -651,6 +682,51 @@ class VolcanoPlannerTest {
}
}
+ static class PhysSingleSubsetRule extends RelOptRule {
+ private final List<String> buf;
+
+ PhysSingleSubsetRule(List<String> buf) {
+ super(operand(PhysSingleRel.class, operand(RelSubset.class, any())));
+ this.buf = buf;
+ }
+
+ @Override public Convention getOutConvention() {
+ return PHYS_CALLING_CONVENTION;
+ }
+
+ @Override public void onMatch(RelOptRuleCall call) {
+ PhysSingleRel singleRel = call.rel(0);
+ RelSubset subset = call.rel(1);
+ buf.add(singleRel.getClass().getSimpleName() + ":"
+ + subset.getDigest());
+ }
+ }
+
+ /**
+ * Create an artificial RelSet merge in the PhysSingleRel's input RelSet
+ */
+ static class PhysSingleInputSetMergeRule extends RelOptRule {
+
+ PhysSingleInputSetMergeRule() {
+ super(
+ operand(PhysSingleRel.class,
+ operand(PhysLeafRel.class, PHYS_CALLING_CONVENTION, any())));
+ }
+
+ @Override public void onMatch(RelOptRuleCall call) {
+ PhysSingleRel singleRel = call.rel(0);
+ PhysLeafRel input = call.rel(1);
+ RelNode newInput =
+ new PhysLeafRel(input.getCluster(), PHYS_CALLING_CONVENTION_3, "a");
+
+ VolcanoPlanner planner = (VolcanoPlanner) call.getPlanner();
+ // Register into a new RelSet first
+ planner.ensureRegistered(newInput, null);
+ // Merge into the old RelSet
+ planner.ensureRegistered(newInput, input);
+ }
+ }
+
// NOTE: Previously, ReformedSingleRule didn't work because it explicitly
// specifies PhysLeafRel rather than RelNode for the single input. Since
// the PhysLeafRel is in a different subset from the original NoneLeafRel,
