morrySnow commented on code in PR #13681:
URL: https://github.com/apache/doris/pull/13681#discussion_r1015133792
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/InnerJoinLAsscomProject.java:
##########
@@ -111,14 +117,26 @@ public Rule build() {
}
}
// replace hashJoinConjuncts
- newBottomHashJoinConjuncts =
JoinUtils.replaceHashConjuncts(
+ newBottomHashJoinConjuncts =
JoinUtils.replaceJoinConjuncts(
newBottomHashJoinConjuncts, outputToInput);
- newTopHashJoinConjuncts = JoinUtils.replaceHashConjuncts(
+ newTopHashJoinConjuncts = JoinUtils.replaceJoinConjuncts(
newTopHashJoinConjuncts, inputToOutput);
- // Add all slots used by hashOnCondition when projects not
empty.
- // TODO: Does nonHashOnCondition also need to be
considered.
- Map<Boolean, Set<Slot>> abOnUsedSlots =
newTopHashJoinConjuncts.stream()
+ // replace otherJoinConjuncts
+ newBottomOtherJoinConjuncts =
JoinUtils.replaceJoinConjuncts(
+ newBottomOtherJoinConjuncts, outputToInput);
+ newTopOtherJoinConjuncts = JoinUtils.replaceJoinConjuncts(
+ newTopOtherJoinConjuncts, inputToOutput);
+
+ if (newBottomHashJoinConjuncts == null ||
newTopHashJoinConjuncts == null
+ || newBottomOtherJoinConjuncts == null ||
newTopOtherJoinConjuncts == null) {
Review Comment:
why prohibit other join conjuncts is null
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/InnerJoinLAsscom.java:
##########
@@ -52,34 +52,38 @@ public class InnerJoinLAsscom extends
OneExplorationRuleFactory {
public Rule build() {
return innerLogicalJoin(innerLogicalJoin(), group())
.when(topJoin -> checkReorder(topJoin, topJoin.left()))
- // TODO: handle otherJoinCondition
- .when(topJoin -> topJoin.getOtherJoinConjuncts().isEmpty())
- .when(topJoin ->
topJoin.left().getOtherJoinConjuncts().isEmpty())
.then(topJoin -> {
LogicalJoin<GroupPlan, GroupPlan> bottomJoin =
topJoin.left();
GroupPlan a = bottomJoin.left();
GroupPlan b = bottomJoin.right();
GroupPlan c = topJoin.right();
// split HashJoinConjuncts.
- Map<Boolean, List<Expression>> splitOn =
splitHashConjuncts(topJoin.getHashJoinConjuncts(),
- bottomJoin);
- List<Expression> newTopHashConjuncts = splitOn.get(true);
- List<Expression> newBottomHashConjuncts =
splitOn.get(false);
+ Map<Boolean, List<Expression>> splitHashConjunts =
splitConjuncts(topJoin.getHashJoinConjuncts(),
+ bottomJoin, bottomJoin.getHashJoinConjuncts());
+ List<Expression> newTopHashConjuncts =
splitHashConjunts.get(true);
+ List<Expression> newBottomHashConjuncts =
splitHashConjunts.get(false);
+ Preconditions.checkState(!newTopHashConjuncts.isEmpty(),
+ "LAsscom newTopHashJoinConjuncts join can't
empty");
if (newBottomHashConjuncts.size() == 0) {
return null;
}
- // TODO: split otherCondition.
+ // split OtherJoinConjuncts.
+ Map<Boolean, List<Expression>> splitOtherConjunts =
splitConjuncts(topJoin.getOtherJoinConjuncts(),
+ bottomJoin, bottomJoin.getOtherJoinConjuncts());
+ List<Expression> newTopOtherConjuncts =
splitOtherConjunts.get(true);
+ List<Expression> newBottomOtherConjuncts =
splitOtherConjunts.get(false);
Review Comment:
chould these two list be **null**?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/OuterJoinLAsscomProject.java:
##########
@@ -117,14 +131,25 @@ public Rule build() {
}
}
// replace hashJoinConjuncts
- newBottomHashJoinConjuncts =
JoinUtils.replaceHashConjuncts(
+ newBottomHashJoinConjuncts =
JoinUtils.replaceJoinConjuncts(
newBottomHashJoinConjuncts, outputToInput);
- newTopHashJoinConjuncts = JoinUtils.replaceHashConjuncts(
+ newTopHashJoinConjuncts = JoinUtils.replaceJoinConjuncts(
newTopHashJoinConjuncts, inputToOutput);
+ // replace otherJoinConjuncts
+ newBottomOtherJoinConjuncts =
JoinUtils.replaceJoinConjuncts(
+ newBottomOtherJoinConjuncts, outputToInput);
+ newTopOtherJoinConjuncts = JoinUtils.replaceJoinConjuncts(
+ newTopOtherJoinConjuncts, inputToOutput);
+
+ if (newBottomHashJoinConjuncts == null ||
newTopHashJoinConjuncts == null
+ || newBottomOtherJoinConjuncts == null ||
newTopOtherJoinConjuncts == null) {
+ return null;
Review Comment:
other conjuncts could be null, and it is ok i think
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/InnerJoinLAsscomProject.java:
##########
@@ -172,15 +192,17 @@ public static Set<ExprId> getExprIdSetForB(GroupPlan b,
List<NamedExpression> bP
}
/**
- * Split HashCondition into two part.
+ * Split Condition into two part.
+ * True: contains B.
+ * False: just contains A C.
*/
- public static Map<Boolean, List<Expression>>
splitHashConjunctsWithAlias(List<Expression> topHashConjuncts,
- LogicalJoin<GroupPlan, GroupPlan> bottomJoin, Set<ExprId>
bExprIdSet) {
+ public static Map<Boolean, List<Expression>>
splitConjunctsWithAlias(List<Expression> topConjuncts,
+ LogicalJoin<GroupPlan, GroupPlan> bottomJoin, List<Expression>
bottomConjunct, Set<ExprId> bExprIdSet) {
Review Comment:
```suggestion
LogicalJoin<GroupPlan, GroupPlan> bottomJoin, List<Expression>
bottomConjuncts, Set<ExprId> bExprIdSet) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/OuterJoinLAsscomProject.java:
##########
@@ -89,20 +87,36 @@ 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");
+ // When newTopHashJoinConjuncts.size() !=
bottomJoin.getHashJoinConjuncts().size()
+ // It means that topHashJoinConjuncts contain A, B, C, we
should LAsscom.
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(
Review Comment:
if splitConjunctsWithAlias used both for Inner and Outer, it should be move
to JoinUtils
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/OuterJoinLAsscomProject.java:
##########
@@ -89,20 +87,36 @@ 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");
+ // When newTopHashJoinConjuncts.size() !=
bottomJoin.getHashJoinConjuncts().size()
+ // It means that topHashJoinConjuncts contain A, B, C, we
should LAsscom.
Review Comment:
```suggestion
// It means that topHashJoinConjuncts contain A, B, C,
we should not do LAsscom.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
