[GitHub] [calcite] hbtoo commented on a change in pull request #1260: [CALCITE-3118] fix VolcanoRuleCall match parent child ordinal check

[GitBox <gi...@apache.org>]

hbtoo commented on a change in pull request #1260: [CALCITE-3118] fix VolcanoRuleCall match parent child ordinal check
URL: https://github.com/apache/calcite/pull/1260#discussion_r291593237
 
 

 ##########
 File path: core/src/test/java/org/apache/calcite/plan/volcano/VolcanoPlannerTest.java
 ##########
 @@ -122,6 +125,35 @@ public VolcanoPlannerTest() {
     assertTrue(result instanceof PhysSingleRel);
   }
 
+  @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);
+
 
 Review comment:
   Thanks @danny0405 for reviewing! Having two relNodes of the same class in two RelSubsets in the same RelSet (thus the equivalent line) is the necessary condition to trigger this bug. However, using convention in the unit test is not a must. It's merely to produce the two subSets. Any two traits that are different would suffice. 
   
   Also as in AssertOperandsDifferentRule, it is not trying to match any convention. We are simply trying to match a parent with its first and second children being PhysLeafRel.class, it is wrong to return a match with a parent relNode and both its second children.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services