[GitHub] [calcite] hsyuan commented on a change in pull request #2079: [CALCITE-4126] Stackoverflow error when applying JoinCommuteRule (Liya Fan)

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

hsyuan commented on a change in pull request #2079:
URL: https://github.com/apache/calcite/pull/2079#discussion_r460613739



##########
File path: core/src/main/java/org/apache/calcite/rel/rules/JoinCommuteRule.java
##########
@@ -59,39 +61,63 @@
   @Deprecated // to be removed before 1.25
   public static final JoinCommuteRule SWAP_OUTER = CoreRules.JOIN_COMMUTE_OUTER;
 
+  /**
+   * This is just a sample implementation, with little practical value.
+   */
+  private static final BiPredicate<RelNode, RelNode> DEFAULT_JOIN_CHILDREN_PREDICATE =
+      (left, right) -> left.getId() < right.getId();
+
   private final boolean swapOuter;
 
+  private final BiPredicate<RelNode, RelNode> joinChildrenPredicate;
+
   //~ Constructors -----------------------------------------------------------
 
   /**
    * Creates a JoinCommuteRule.
+   * @param clazz class for the join.
+   * @param relBuilderFactory RelNode build factory.
+   * @param swapOuter whether outer joins are supported.
+   * @param joinChildrenPredicate a predicate for the left and right children of the join.
+   *               It checks if it is of value to swap the left and right child of the join,
+   *               so the swap should happen only if the predicate yields true.The
+   *               predicate must form a partial order. That means for different a and b,
+   *               if pred(a, b) = true, then we must have pred(b, a) = false; and vice versa.
+   *               Otherwise, unexpected behaviors may occur, including endless
+   *               firing of the rule.
    */
   public JoinCommuteRule(Class<? extends Join> clazz,
-      RelBuilderFactory relBuilderFactory, boolean swapOuter) {
+                         RelBuilderFactory relBuilderFactory, boolean swapOuter,
+                         BiPredicate<RelNode, RelNode> joinChildrenPredicate) {
     // FIXME Enable this rule for joins with system fields
     super(
         operandJ(clazz, null,
             (Predicate<Join>) j -> j.getLeft().getId() != j.getRight().getId()
                 && j.getSystemFieldList().isEmpty(),
-            any()),
+            operandJ(RelNode.class, null, left -> true, any()),
+            operandJ(RelNode.class, null, right -> true, any())),

Review comment:
       This will increase the number of rule matches dramatically.




----------------------------------------------------------------
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