You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/10/31 10:44:44 UTC
[GitHub] [doris] morrySnow commented on a diff in pull request #13681: [enhancement](Nereids) support otherJoinConjuncts in cascades join reorder
morrySnow commented on code in PR #13681:
URL: https://github.com/apache/doris/pull/13681#discussion_r1009140690
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/JoinUtils.java:
##########
@@ -285,28 +286,28 @@ public static boolean couldColocateJoin(DistributionSpecHash leftHashSpec, Distr
}
/**
- * replace hashJoinConjuncts by using slots map.
- * TODO: just support `col1=col2`
+ * replace JoinConjuncts by using slots map.
+ * TODO: just support `col1 [cmp = > < ...] col2`
*/
- public static List<Expression> replaceHashConjuncts(List<Expression> hashJoinConjuncts,
+ public static List<Expression> replaceJoinConjuncts(List<Expression> hashJoinConjuncts,
Map<Slot, Slot> replaceMaps) {
List<Expression> newHashJoinConjuncts = Lists.newArrayList();
for (Expression hashJoinConjunct : hashJoinConjuncts) {
- if (!(hashJoinConjunct instanceof EqualTo)) {
+ if (!(hashJoinConjunct instanceof ComparisonPredicate)) {
return null;
}
- EqualTo equalTo = (EqualTo) hashJoinConjunct;
- if (!(equalTo.left() instanceof Slot) || !(equalTo.right() instanceof Slot)) {
+ ComparisonPredicate cmp = (ComparisonPredicate) hashJoinConjunct;
+ if (!(cmp.left() instanceof Slot) || !(cmp.right() instanceof Slot)) {
Review Comment:
when we process other conditions on join. we could meet the expression such as `a.c1 + a.c2 > b.c3`. So we cannot process other join condition with this constraint
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/InnerJoinLAsscom.java:
##########
@@ -93,13 +97,13 @@ public static boolean checkReorder(LogicalJoin<? extends Plan, GroupPlan> topJoi
}
/**
- * Split HashCondition into two part.
+ * Split onCondition into two part.
*/
- public static Map<Boolean, List<Expression>> splitHashConjuncts(List<Expression> topHashConjuncts,
- LogicalJoin<GroupPlan, GroupPlan> bottomJoin) {
+ private static Map<Boolean, List<Expression>> splitConjuncts(List<Expression> topConjuncts,
+ LogicalJoin<GroupPlan, GroupPlan> bottomJoin, List<Expression> bottomConjuncts) {
// top: (A B)(error) (A C) (B C) (A B C)
// Split topJoin hashCondition to two part according to include B.
- Map<Boolean, List<Expression>> splitOn = topHashConjuncts.stream()
+ Map<Boolean, List<Expression>> splitOn = topConjuncts.stream()
.collect(Collectors.partitioningBy(topHashOn -> {
Set<Slot> usedSlot = topHashOn.collect(Slot.class::isInstance);
Review Comment:
nit: could use getInputSlots function in Expression
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/OuterJoinLAsscomProject.java:
##########
@@ -89,20 +87,34 @@ public Rule build() {
Set<ExprId> bExprIdSet = InnerJoinLAsscomProject.getExprIdSetForB(bottomJoin.right(),
newRightProjects);
- /* ********** split HashConjuncts ********** */
- Map<Boolean, List<Expression>> splitOn = InnerJoinLAsscomProject.splitHashConjunctsWithAlias(
- topJoin.getHashJoinConjuncts(), bottomJoin, bExprIdSet);
- List<Expression> newTopHashJoinConjuncts = splitOn.get(true);
+ /* ********** split Conjuncts ********** */
+ Map<Boolean, List<Expression>> splitHashJoinConjuncts
+ = InnerJoinLAsscomProject.splitConjunctsWithAlias(
+ topJoin.getHashJoinConjuncts(), bottomJoin, bottomJoin.getHashJoinConjuncts(), bExprIdSet);
+ List<Expression> newTopHashJoinConjuncts = splitHashJoinConjuncts.get(true);
+ Preconditions.checkState(!newTopHashJoinConjuncts.isEmpty(),
+ "LAsscom newTopHashJoinConjuncts join can't empty");
if (topJoin.getJoinType() != bottomJoin.getJoinType()
&& newTopHashJoinConjuncts.size() != bottomJoin.getHashJoinConjuncts().size()) {
return null;
}
- List<Expression> newBottomHashJoinConjuncts = splitOn.get(false);
+ List<Expression> newBottomHashJoinConjuncts = splitHashJoinConjuncts.get(false);
if (newBottomHashJoinConjuncts.size() == 0) {
return null;
}
- /* ********** replace HashConjuncts by projects ********** */
+ Map<Boolean, List<Expression>> splitOtherJoinConjuncts
+ = InnerJoinLAsscomProject.splitConjunctsWithAlias(
+ topJoin.getOtherJoinConjuncts(), bottomJoin, bottomJoin.getOtherJoinConjuncts(),
+ bExprIdSet);
+ List<Expression> newTopOtherJoinConjuncts = splitOtherJoinConjuncts.get(true);
+ if (topJoin.getJoinType() != bottomJoin.getJoinType()
+ && newTopOtherJoinConjuncts.size() != bottomJoin.getOtherJoinConjuncts().size()) {
+ return null;
Review Comment:
add comment to explain why~
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/exploration/join/InnerJoinLAsscomProjectTest.java:
##########
@@ -127,4 +132,44 @@ void testAliasTopMultiHashJoin() {
).when(join -> join.getHashJoinConjuncts().size() == 2)
);
}
+
+ @Test
+ public void testHashAndOther() {
+ List<Expression> bottomHashJoinConjunct = ImmutableList.of(
Review Comment:
we need add some complicate expressions in other condition test.
```
a.c1 + a.c2 > b.c1
a.c1 + b.c1 > 0
substring(a.c1, 5, 10) != '123456'
...
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/InnerJoinLAsscom.java:
##########
@@ -93,13 +97,13 @@ public static boolean checkReorder(LogicalJoin<? extends Plan, GroupPlan> topJoi
}
/**
- * Split HashCondition into two part.
+ * Split onCondition into two part.
Review Comment:
add more comment, to explain which expression will split into true part and which expression will split into the false part
--
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@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org