vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r593809931
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,41 @@ private AggregateOnCalcToAggregateUnifyRule() {
}
}
+ /** A {@link SubstitutionVisitor.UnifyRule} that matches a
+ * {@link org.apache.calcite.rel.core.Join} to a
+ * {@link org.apache.calcite.rel.core.Join}, provided
+ * that they have the same child. */
+ private static class JoinToJoinUnifyRule extends AbstractUnifyRule {
+ public static final JoinToJoinUnifyRule INSTANCE = new
JoinToJoinUnifyRule();
+ private JoinToJoinUnifyRule() {
+ super(operand(MutableJoin.class, query(0)),
+ operand(MutableJoin.class, target(0)), 1);
+ }
+
+ @Override protected @Nullable UnifyResult apply(UnifyRuleCall call) {
+ MutableJoin query = (MutableJoin) call.query;
+ MutableJoin target = (MutableJoin) call.target;
+
+ // same join type
+ final JoinRelType joinRelType = sameJoinType(query.joinType,
target.joinType);
+ if (joinRelType == null) {
+ return null;
+ }
+ List<RexNode> queryCondition = RelOptUtil.conjunctions(query.condition);
+ List<RexNode> targetCondition =
RelOptUtil.conjunctions(target.condition);
+ // same join condition
+ if (queryCondition.size() == targetCondition.size()) {
+ Set<RexNode> queryConditionDigest =
queryCondition.stream().collect(Collectors.toSet());
+ Set<RexNode> targetConditionDigest =
targetCondition.stream().collect(Collectors.toSet());
Review comment:
This looks suspicious.
Do you think `List<RexNode> queryCondition` can contain duplicate values?
Do you think `List<RexNode> targetCondition` can contain duplicate values?
a) If duplicates are possible, then you should not compare lists by size.
You need to deduplicate elements, and only then compare sets.
b) If you think duplicates are not possible, then you do not need to convert
the second list to set, and use `set.containsAll(list)`.
In any case, it looks like either `if size=size` should be removed or one of
the sets should be removed.
WDYT?
----------------------------------------------------------------
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:
[email protected]
