You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by da...@apache.org on 2019/07/24 06:10:59 UTC
[calcite] branch master updated: [CALCITE-3118] VolcanoRuleCall
should look at RelSubset rather than RelSet when checking child ordinal of
a parent operand (Botong Huang)
This is an automated email from the ASF dual-hosted git repository.
danny0405 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 2d52dd3 [CALCITE-3118] VolcanoRuleCall should look at RelSubset rather than RelSet when checking child ordinal of a parent operand (Botong Huang)
2d52dd3 is described below
commit 2d52dd3f2dc3e6087c034008037e6c47b201c732
Author: Botong Huang <bo...@apache.org>
AuthorDate: Fri Jun 7 20:57:30 2019 -0700
[CALCITE-3118] VolcanoRuleCall should look at RelSubset rather than RelSet when checking child ordinal of a parent operand (Botong Huang)
When matching in ascending order, only the parent operands in input
RelSubSet should be considered.
Fix-ups:(Danny Chan)
- Remove TestBiRel because it is only used by PhysBiRel
- Add issue link to the test case
close apache/calcite#1260
---
.../calcite/plan/volcano/VolcanoRuleCall.java | 2 +-
.../apache/calcite/plan/volcano/PlannerTests.java | 67 +++++++++++++++++++++-
.../calcite/plan/volcano/VolcanoPlannerTest.java | 36 ++++++++++++
3 files changed, 102 insertions(+), 3 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
index 5aca507..45f5cfc 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
@@ -335,7 +335,7 @@ public class VolcanoRuleCall extends RelOptRuleCall {
// but now check that it is the *correct* child.
final RelSubset input =
(RelSubset) rel.getInput(previousOperand.ordinalInParent);
- List<RelNode> inputRels = input.set.getRelsFromAllSubsets();
+ List<RelNode> inputRels = input.getRelList();
if (!inputRels.contains(previous)) {
continue;
}
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 713e5c1..33ac7ba 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
@@ -24,6 +24,7 @@ import org.apache.calcite.plan.RelOptRule;
import org.apache.calcite.plan.RelOptRuleCall;
import org.apache.calcite.plan.RelTraitSet;
import org.apache.calcite.rel.AbstractRelNode;
+import org.apache.calcite.rel.BiRel;
import org.apache.calcite.rel.RelNode;
import org.apache.calcite.rel.RelWriter;
import org.apache.calcite.rel.SingleRel;
@@ -57,6 +58,18 @@ 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 RelOptCluster newCluster(VolcanoPlanner planner) {
final RelDataTypeFactory typeFactory =
new SqlTypeFactoryImpl(org.apache.calcite.rel.type.RelDataTypeSystem.DEFAULT);
@@ -117,6 +130,29 @@ class PlannerTests {
}
}
+ /** Relational expression with two inputs and convention PHYS. */
+ static class PhysBiRel extends BiRel {
+ PhysBiRel(RelOptCluster cluster, RelTraitSet traitSet, RelNode left,
+ RelNode right) {
+ super(cluster, traitSet, left, right);
+ }
+
+ @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
+ RelMetadataQuery mq) {
+ return planner.getCostFactory().makeTinyCost();
+ }
+
+ @Override public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs) {
+ assert inputs.size() == 2;
+ return new PhysBiRel(getCluster(), traitSet, inputs.get(0),
+ inputs.get(1));
+ }
+
+ @Override protected RelDataType deriveRowType() {
+ return getLeft().getRowType();
+ }
+ }
+
/** Relational expression with zero inputs and convention NONE. */
static class NoneLeafRel extends TestLeafRel {
NoneLeafRel(RelOptCluster cluster, String label) {
@@ -132,8 +168,15 @@ class PlannerTests {
/** Relational expression with zero inputs and convention PHYS. */
static class PhysLeafRel extends TestLeafRel {
+ Convention convention;
+
PhysLeafRel(RelOptCluster cluster, String label) {
- super(cluster, cluster.traitSetOf(PHYS_CALLING_CONVENTION), label);
+ this(cluster, PHYS_CALLING_CONVENTION, label);
+ }
+
+ PhysLeafRel(RelOptCluster cluster, Convention convention, String label) {
+ super(cluster, cluster.traitSetOf(convention), label);
+ this.convention = convention;
}
@Override public RelOptCost computeSelfCost(RelOptPlanner planner,
@@ -142,7 +185,7 @@ class PlannerTests {
}
@Override public RelNode copy(RelTraitSet traitSet, List<RelNode> inputs) {
- assert traitSet.comprises(PHYS_CALLING_CONVENTION);
+ assert traitSet.comprises(convention);
assert inputs.isEmpty();
return this;
}
@@ -202,6 +245,26 @@ class PlannerTests {
new PhysSingleRel(single.getCluster(), physInput));
}
}
+
+ /**
+ * Planner rule that matches a parent with two children and asserts that they
+ * are not the same.
+ */
+ static class AssertOperandsDifferentRule extends RelOptRule {
+ AssertOperandsDifferentRule() {
+ super(
+ operand(PhysBiRel.class,
+ operand(PhysLeafRel.class, any()),
+ operand(PhysLeafRel.class, any())));
+ }
+
+ public void onMatch(RelOptRuleCall call) {
+ PhysLeafRel left = call.rel(1);
+ PhysLeafRel right = call.rel(2);
+
+ assert left != right : left + " should be different from " + right;
+ }
+ }
}
// End PlannerTests.java
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 d2349d7..781d4b2 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
@@ -44,10 +44,13 @@ import java.util.Arrays;
import java.util.Collections;
import java.util.List;
+import static org.apache.calcite.plan.volcano.PlannerTests.AssertOperandsDifferentRule;
import static org.apache.calcite.plan.volcano.PlannerTests.GoodSingleRule;
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.PhysBiRel;
import static org.apache.calcite.plan.volcano.PlannerTests.PhysLeafRel;
import static org.apache.calcite.plan.volcano.PlannerTests.PhysLeafRule;
import static org.apache.calcite.plan.volcano.PlannerTests.PhysSingleRel;
@@ -122,6 +125,39 @@ public class VolcanoPlannerTest {
assertTrue(result instanceof PhysSingleRel);
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-3118">[CALCITE-3118]
+ * VolcanoRuleCall should look at RelSubset rather than RelSet
+ * when checking child ordinal of a parent operand</a>. */
+ @Test public void testMatchedOperandsDifferent() {
+ VolcanoPlanner planner = new VolcanoPlanner();
+ planner.addRelTraitDef(ConventionTraitDef.INSTANCE);
+ RelOptCluster cluster = newCluster(planner);
+
+ // The rule that triggers the assert rule
+ planner.addRule(new PhysLeafRule());
+
+ // The rule asserting that the matched operands are different
+ planner.addRule(new AssertOperandsDifferentRule());
+
+ // Construct two children in the same set and a parent RelNode
+ NoneLeafRel leftRel = new NoneLeafRel(cluster, "a");
+ RelNode leftPhy = planner
+ .changeTraits(leftRel, cluster.traitSetOf(PHYS_CALLING_CONVENTION));
+ PhysLeafRel rightPhy =
+ new PhysLeafRel(cluster, PHYS_CALLING_CONVENTION_2, "b");
+
+ PhysBiRel parent =
+ new PhysBiRel(cluster, cluster.traitSetOf(PHYS_CALLING_CONVENTION),
+ leftPhy, rightPhy);
+ planner.setRoot(parent);
+
+ // Make sure both RelNodes are in the same set, but different subset
+ planner.ensureRegistered(leftPhy, rightPhy);
+
+ planner.chooseDelegate().findBestExp();
+ }
+
/**
* Tests a rule that is fired once per subset (whereas most rules are fired
* once per rel in a set or rel in a subset)