morrySnow commented on code in PR #11812:
URL: https://github.com/apache/doris/pull/11812#discussion_r951158836
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/join/JoinLAsscomHelper.java:
##########
@@ -165,7 +166,8 @@ private Pair<List<NamedExpression>, List<NamedExpression>>
getProjectExprs() {
private LogicalJoin<GroupPlan, GroupPlan> newBottomJoin() {
return new LogicalJoin(
bottomJoin.getJoinType(),
- Optional.of(ExpressionUtils.and(newBottomJoinOnCondition)),
+ newBottomJoinOnCondition,
+ Optional.empty(),
Review Comment:
why all other conjuncts are empty? do we will lose some expression on other
conjuncts such as t1.a > t2.b?
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/exploration/join/JoinLAsscomProjectTest.java:
##########
@@ -88,10 +88,12 @@ private Pair<LogicalJoin, LogicalJoin>
testJoinProjectLAsscom(List<NamedExpressi
LogicalProject<LogicalJoin<LogicalOlapScan, LogicalOlapScan>> project
= new LogicalProject<>(
projects,
- new LogicalJoin<>(JoinType.INNER_JOIN,
Optional.of(bottomJoinOnCondition), scans.get(0), scans.get(1)));
+ new LogicalJoin<>(JoinType.INNER_JOIN,
Lists.newArrayList(bottomJoinOnCondition),
+ Optional.empty(), scans.get(0), scans.get(1)));
Review Comment:
we need to add case that join has other conjuncts
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJoin.java:
##########
@@ -132,7 +165,10 @@ public List<Slot> computeOutput(Plan leftInput, Plan
rightInput) {
@Override
public String toString() {
StringBuilder sb = new StringBuilder("LogicalJoin (").append(joinType);
- condition.ifPresent(expression -> sb.append(", ").append(expression));
+ sb.append(" [");
+ hashJoinConjuncts.stream().map(expr -> sb.append("
").append(expr)).collect(Collectors.toList());
Review Comment:
please refer to PhysicalJoin's toString's comment
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/MultiJoin.java:
##########
@@ -124,20 +140,27 @@ private Plan reorderJoinsAccordingToConditions(List<Plan>
joinInputs, List<Expre
}).findFirst();
Plan right = rightOpt.orElseGet(() -> candidate.get(1));
- Set<Slot> joinOutput = getJoinOutput(left, right);
- Map<Boolean, List<Expression>> split = splitConjuncts(conjuncts,
joinOutput);
- List<Expression> joinConditions = split.get(true);
- List<Expression> nonJoinConditions = split.get(false);
-
- Optional<Expression> cond;
+ Pair<List<Expression>, List<Expression>> pair =
JoinUtils.extractExpressionForHashTable(
+
joinInputs.get(0).getOutput().stream().map(SlotReference.class::cast).collect(Collectors.toList()),
+
joinInputs.get(1).getOutput().stream().map(SlotReference.class::cast).collect(Collectors.toList()),
+ conjuncts);
+ //Set<Slot> joinOutput = getJoinOutput(left, right);
+ //Map<Boolean, List<Expression>> split = splitConjuncts(conjuncts,
joinOutput);
+ //List<Expression> joinConditions = split.get(true);
+ //List<Expression> nonJoinConditions = split.get(false);
Review Comment:
remove it
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/exploration/join/JoinLAsscomTest.java:
##########
@@ -73,9 +73,11 @@ public Pair<LogicalJoin, LogicalJoin> testJoinLAsscom(
*/
Assertions.assertEquals(3, scans.size());
LogicalJoin<LogicalOlapScan, LogicalOlapScan> bottomJoin = new
LogicalJoin<>(JoinType.INNER_JOIN,
- Optional.of(bottomJoinOnCondition), scans.get(0),
scans.get(1));
+ Lists.newArrayList(bottomJoinOnCondition),
Review Comment:
we need to add case that join has other conjuncts
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalHashJoin.java:
##########
@@ -65,7 +67,10 @@ public <R, C> R accept(PlanVisitor<R, C> visitor, C context)
{
public String toString() {
StringBuilder sb = new StringBuilder();
sb.append("PhysicalHashJoin ([").append(joinType).append("]");
- condition.ifPresent(
+ sb.append(" [");
+ hashJoinConjuncts.stream().map(expr -> sb.append("
").append(expr)).collect(Collectors.toList());
+ sb.append(" ]");
Review Comment:
```suggestion
sb.append(Strings.join(hashJoinConjuncts, ", "));
sb.append(" ]");
```
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/exploration/join/JoinCommuteTest.java:
##########
@@ -47,7 +48,8 @@ public void testInnerJoinCommute() {
new SlotReference("id", new BigIntType(), true,
ImmutableList.of("table1")),
new SlotReference("id", new BigIntType(), true,
ImmutableList.of("table2")));
LogicalJoin<LogicalOlapScan, LogicalOlapScan> join = new LogicalJoin<>(
- JoinType.INNER_JOIN, Optional.of(onCondition), scan1, scan2);
+ JoinType.INNER_JOIN, Lists.newArrayList(onCondition),
Review Comment:
we need to add case that join has other conjuncts
--
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-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
