You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/03/09 08:08:41 UTC
[GitHub] [calcite] xy2953396112 opened a new pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
xy2953396112 opened a new pull request #2365:
URL: https://github.com/apache/calcite/pull/2365
https://issues.apache.org/jira/browse/CALCITE-4391
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590066131
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
+ // same child
+ if (query.getLeft().equals(target.getLeft()) && query.getRight().equals(target.getRight())) {
Review comment:
Should this be in `matches`?
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#issuecomment-793523499
BTW, please help to review PR, thanks a lot.
[1] [Add an interface in RelOptMaterializations to allow registering UnifyRule](https://github.com/apache/calcite/pull/2262)
[2] [Add an interface in RelOptMaterializations to allow registering UnifyRule](https://github.com/apache/calcite/pull/2094)
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r593178914
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
Review comment:
Please move the condition to `matches` method. I do not understand why you mark this as resolved since the issue is still there.
----------------------------------------------------------------
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
[GitHub] [calcite] chunweilei commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
chunweilei commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r594020056
##########
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:
+1 for @vlsi
##########
File path: core/src/test/java/org/apache/calcite/materialize/NormalizationTrimFieldTest.java
##########
@@ -106,4 +116,41 @@
final String relOptimizedStr = RelOptUtil.toString(relOptimized.get(0).getKey());
assertThat(isLinux(optimized).matches(relOptimizedStr), is(true));
}
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-4391">[CALCITE-4391]
+ * The order of join conditions is different,materialized view recognition fails</a>. */
+ @Test void testJoinToJoinConditionReorder() {
+ final RelBuilder relBuilder = RelBuilder.create(config().build());
Review comment:
The order of join conditions is different, materialized view recognition fails -> materialized view recognition fails when the order of join conditions is different?
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590152508
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
Review comment:
The matching condition is that they have the same join type.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590065715
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
Review comment:
Please move the precondition to `matches` method
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
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?
Just in case, I have absolutely no clue.
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:
users@infra.apache.org
[GitHub] [calcite] aigor commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
aigor commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590389195
##########
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##########
@@ -25,6 +25,7 @@
/**
* Inner join.
*/
+
Review comment:
I think that this change is unnecessary.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#issuecomment-797491945
@vlsi
Please help to review again, thanks.
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590076768
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
Review comment:
thanks for review. If query has different `joinType` with target, the `matches` method can not judge the type of `join`.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590089514
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,43 @@ 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 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<String> queryConditionDigest = queryCondition.stream()
+ .map(rex -> rex.toString()).collect(Collectors.toSet());
+ Set<String> targetConditionDigest = targetCondition.stream()
+ .map(rex -> rex.toString()).collect(Collectors.toSet());
Review comment:
Please use `RexNode#equals` rather than `toString` (see https://issues.apache.org/jira/browse/CALCITE-3786)
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590077286
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
+ // same child
+ if (query.getLeft().equals(target.getLeft()) && query.getRight().equals(target.getRight())) {
Review comment:
It's not necessary, thx.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r593725539
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
Review comment:
Ah, indeed I confused `AbstractUnifyRule` with `RelOptRule`. There's no problem then
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590152726
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,43 @@ 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 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<String> queryConditionDigest = queryCondition.stream()
+ .map(rex -> rex.toString()).collect(Collectors.toSet());
+ Set<String> targetConditionDigest = targetCondition.stream()
+ .map(rex -> rex.toString()).collect(Collectors.toSet());
Review comment:
ok
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 closed pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 closed pull request #2365:
URL: https://github.com/apache/calcite/pull/2365
--
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.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590090275
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
Review comment:
How does that impact `matches` method?
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r593178394
##########
File path: core/src/test/java/org/apache/calcite/materialize/NormalizationTrimFieldTest.java
##########
@@ -106,4 +116,38 @@
final String relOptimizedStr = RelOptUtil.toString(relOptimized.get(0).getKey());
assertThat(isLinux(optimized).matches(relOptimizedStr), is(true));
}
+
+ @Test void testJoinToJoinConditionReorder() {
+ final RelBuilder relBuilder = RelBuilder.create(config().build());
+ final RelNode query =
+ relBuilder.scan("EMP")
+ .scan("DEPT")
+ .join(JoinRelType.INNER,
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "DEPTNO"),
+ relBuilder.field(2, "DEPT", "DEPTNO")),
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "ENAME"),
+ relBuilder.field(2, "DEPT", "DNAME"))).project(relBuilder.field(0)).build();
+
+ final RelNode target =
+ relBuilder.scan("EMP")
+ .scan("DEPT")
+ .join(JoinRelType.INNER,
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "ENAME"),
+ relBuilder.field(2, "DEPT", "DNAME")),
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "DEPTNO"),
+ relBuilder.field(2, "DEPT", "DEPTNO"))).project(relBuilder.field(0)).build();
+
+ final RelNode replacement = relBuilder.scan("mv1").build();
+ final List<RelNode> relOptimized = new SubstitutionVisitor(target, query).go(replacement);
+
+ final String optimized = ""
+ + "LogicalTableScan(table=[[mv1]])\n";
+ final String relOptimizedStr = RelOptUtil.toString(relOptimized.get(0));
+ assertThat(isLinux(optimized).matches(relOptimizedStr), is(true));
Review comment:
Please use `reason` message and matcher appropriately so the failure stacktrace explains the nature of the faile
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r593719336
##########
File path: core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java
##########
@@ -1550,6 +1552,44 @@ 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(any(MutableJoin.class), any(MutableJoin.class), 0);
+ }
+
+ @Override protected 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;
+ }
Review comment:
Do you mean that the usage of the precondition is same as `RelOptRule`?
The usage of the two is different here, you can refer `org.apache.calcite.plan.SubstitutionVisitor.JoinOnLeftCalcToJoinUnifyRule`.
----------------------------------------------------------------
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
[GitHub] [calcite] vlsi commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r596040966
##########
File path: core/src/test/java/org/apache/calcite/materialize/NormalizationTrimFieldTest.java
##########
@@ -106,4 +116,38 @@
final String relOptimizedStr = RelOptUtil.toString(relOptimized.get(0).getKey());
assertThat(isLinux(optimized).matches(relOptimizedStr), is(true));
}
+
+ @Test void testJoinToJoinConditionReorder() {
+ final RelBuilder relBuilder = RelBuilder.create(config().build());
+ final RelNode query =
+ relBuilder.scan("EMP")
+ .scan("DEPT")
+ .join(JoinRelType.INNER,
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "DEPTNO"),
+ relBuilder.field(2, "DEPT", "DEPTNO")),
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "ENAME"),
+ relBuilder.field(2, "DEPT", "DNAME"))).project(relBuilder.field(0)).build();
+
+ final RelNode target =
+ relBuilder.scan("EMP")
+ .scan("DEPT")
+ .join(JoinRelType.INNER,
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "ENAME"),
+ relBuilder.field(2, "DEPT", "DNAME")),
+ relBuilder.call(SqlStdOperatorTable.EQUALS,
+ relBuilder.field(2, "EMP", "DEPTNO"),
+ relBuilder.field(2, "DEPT", "DEPTNO"))).project(relBuilder.field(0)).build();
+
+ final RelNode replacement = relBuilder.scan("mv1").build();
+ final List<RelNode> relOptimized = new SubstitutionVisitor(target, query).go(replacement);
+
+ final String optimized = ""
+ + "LogicalTableScan(table=[[mv1]])\n";
+ final String relOptimizedStr = RelOptUtil.toString(relOptimized.get(0));
+ assertThat(isLinux(optimized).matches(relOptimizedStr), is(true));
Review comment:
Just for the reference, `Matcher#matches` is in the list of forbidden signatures to avoid its accidental use :)
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 commented on a change in pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 commented on a change in pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#discussion_r590409759
##########
File path: core/src/main/java/org/apache/calcite/rel/core/JoinRelType.java
##########
@@ -25,6 +25,7 @@
/**
* Inner join.
*/
+
Review comment:
thanks
----------------------------------------------------------------
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
[GitHub] [calcite] xy2953396112 edited a comment on pull request #2365: [CALCITE-4391] Implement JoinToJoinUnifyRule in SubstitutionVisitor
Posted by GitBox <gi...@apache.org>.
xy2953396112 edited a comment on pull request #2365:
URL: https://github.com/apache/calcite/pull/2365#issuecomment-793523499
BTW, please help to review PR, thanks a lot.
[1] [Add an interface in RelOptMaterializations to allow normalization UnifyRule](https://github.com/apache/calcite/pull/2262)
[2] [Add an interface in RelOptMaterializations to allow registering UnifyRule](https://github.com/apache/calcite/pull/2094)
----------------------------------------------------------------
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