924060929 commented on code in PR #11812:
URL: https://github.com/apache/doris/pull/11812#discussion_r953329100
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/jobs/batch/RewriteJob.java:
##########
@@ -40,13 +41,14 @@ public class RewriteJob extends BatchRulesJob {
public RewriteJob(CascadesContext cascadesContext) {
super(cascadesContext);
ImmutableList<Job> jobs = new ImmutableList.Builder<Job>()
- .add(bottomUpBatch(ImmutableList.of(new
MergeConsecutiveProjects())))
- .add(topDownBatch(ImmutableList.of(new
ExpressionNormalization())))
- .add(topDownBatch(ImmutableList.of(new ReorderJoin())))
- .add(topDownBatch(ImmutableList.of(new
PushPredicateThroughJoin())))
+ // add rules in reverse order due to jobPool is stack.
.add(topDownBatch(ImmutableList.of(new
AggregateDisassemble())))
- .build().reverse(); // reverse due to jobPool is stack.
Review Comment:
we should keep the order as the present makes it easier to view. the reverse
can hide in the job base class
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/FindHashConditionForJoin.java:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.rules.rewrite.logical;
+
+import org.apache.doris.common.Pair;
+import org.apache.doris.nereids.rules.Rule;
+import org.apache.doris.nereids.rules.RuleType;
+import org.apache.doris.nereids.rules.rewrite.OneRewriteRuleFactory;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.plans.logical.LogicalJoin;
+import org.apache.doris.nereids.util.ExpressionUtils;
+import org.apache.doris.nereids.util.JoinUtils;
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * this rule aims to find a conjunct list from on clause expression, which
could
+ * be used to build hash-table.
+ *
+ * For example:
+ * A join B on A.x=B.x and A.y>1 and A.x+1=B.x+B.y and A.z=B.z+A.x and
(A.z=B.z or A.x=B.x)
+ * {A.x=B.x, A.x+1=B.x+B.y} could be used to build hash table,
+ * but {A.y>1, A.z=B.z+A.z, (A.z=B.z or A.x=B.x)} are not.
+ *
+ * CAUTION:
+ * This rule must be applied after BindSlotReference
+ */
+public class FindHashConditionForJoin extends OneRewriteRuleFactory {
+ @Override
+ public Rule build() {
+ return logicalJoin().then(join -> {
+ Pair<List<Expression>, List<Expression>> pair =
JoinUtils.extractExpressionForHashTable(join);
+ List<Expression> hashJoinConjuncts = pair.first;
+ hashJoinConjuncts.addAll(join.getHashJoinConjuncts());
+ List<Expression> otherJoinPredicates = pair.second;
+
+ if (!hashJoinConjuncts.isEmpty()) {
Review Comment:
```java
if (!extractedHashJoinConjuncts.isEmpty()) {
// create new LogicalJoin by the combinedHashJoinJoinConjuincts and
otherJoinPredicates,
// which combinedHashJoinJoinConjuincts = extractedHashJoinConjuncts +
originJoin.getHashJoinConjuncts()
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJoin.java:
##########
@@ -177,18 +213,28 @@ public JoinReorderContext getJoinReorderContext() {
@Override
public LogicalBinary<Plan, Plan> withChildren(List<Plan> children) {
Preconditions.checkArgument(children.size() == 2);
- return new LogicalJoin<>(joinType, condition, children.get(0),
children.get(1), joinReorderContext);
+ return new LogicalJoin<>(joinType, hashJoinConjuncts,
otherJoinCondition, children.get(0), children.get(1));
}
@Override
public Plan withGroupExpression(Optional<GroupExpression> groupExpression)
{
- return new LogicalJoin<>(joinType, condition, groupExpression,
- Optional.of(logicalProperties), left(), right(),
joinReorderContext);
+ return new LogicalJoin<>(joinType, hashJoinConjuncts,
otherJoinCondition, groupExpression,
+ Optional.of(logicalProperties), left(), right());
}
@Override
public Plan withLogicalProperties(Optional<LogicalProperties>
logicalProperties) {
- return new LogicalJoin<>(joinType, condition, Optional.empty(),
logicalProperties, left(), right(),
- joinReorderContext);
+ return new LogicalJoin<>(joinType, hashJoinConjuncts,
otherJoinCondition,
+ Optional.empty(), logicalProperties, left(), right());
+ }
+
+ @Override
+ public LEFT_CHILD_TYPE left() {
+ return (LEFT_CHILD_TYPE) child(0);
+ }
+
+ @Override
+ public RIGHT_CHILD_TYPE right() {
+ return (RIGHT_CHILD_TYPE) child(1);
Review Comment:
is this override method neccesary?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/logical/FindHashConditionForJoin.java:
##########
@@ -0,0 +1,69 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.nereids.rules.rewrite.logical;
+
+import org.apache.doris.common.Pair;
+import org.apache.doris.nereids.rules.Rule;
+import org.apache.doris.nereids.rules.RuleType;
+import org.apache.doris.nereids.rules.rewrite.OneRewriteRuleFactory;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.plans.logical.LogicalJoin;
+import org.apache.doris.nereids.util.ExpressionUtils;
+import org.apache.doris.nereids.util.JoinUtils;
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * this rule aims to find a conjunct list from on clause expression, which
could
+ * be used to build hash-table.
+ *
+ * For example:
+ * A join B on A.x=B.x and A.y>1 and A.x+1=B.x+B.y and A.z=B.z+A.x and
(A.z=B.z or A.x=B.x)
+ * {A.x=B.x, A.x+1=B.x+B.y} could be used to build hash table,
+ * but {A.y>1, A.z=B.z+A.z, (A.z=B.z or A.x=B.x)} are not.
+ *
+ * CAUTION:
+ * This rule must be applied after BindSlotReference
+ */
+public class FindHashConditionForJoin extends OneRewriteRuleFactory {
+ @Override
+ public Rule build() {
+ return logicalJoin().then(join -> {
+ Pair<List<Expression>, List<Expression>> pair =
JoinUtils.extractExpressionForHashTable(join);
+ List<Expression> hashJoinConjuncts = pair.first;
+ hashJoinConjuncts.addAll(join.getHashJoinConjuncts());
+ List<Expression> otherJoinPredicates = pair.second;
+
+ if (!hashJoinConjuncts.isEmpty()) {
+ Optional<Expression> condition = Optional.empty();
+ if (!otherJoinPredicates.isEmpty()) {
+ condition =
Optional.of(ExpressionUtils.and(otherJoinPredicates));
+ }
+ return new LogicalJoin(join.getJoinType(),
+ hashJoinConjuncts,
+ condition,
+ Optional.empty(),
+ Optional.empty(),
+ join.left(), join.right());
+ } else {
+ return join;
+ }
Review Comment:
```java
Pair<List<Expression>, List<Expression>> pair =
JoinUtils.extractExpressionForHashTable(join);
List<Expression> extractedHashJoinConjuncts = pair.first;
List<Expression> remaindNonHashJoinConjuncts = pair.second;
if (!hashJoinConjuncts.isEmpty()) {
List<Expression> combinedHashJoinConjuncts = ImmutableList.builder()
.addAll(join.getHashJoinConjuncts())
.addAll(extractedHashJoinConjuncts)
.build();
return new LogicalJoin(join.getJoinType(),
combinedHashJoinConjuncts,
remaindNonHashJoinConjuncts,
Optional.empty(),
Optional.empty(),
join.left(), join.right());
} else {
return join;
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJoin.java:
##########
@@ -43,7 +45,8 @@
extends LogicalBinary<LEFT_CHILD_TYPE, RIGHT_CHILD_TYPE> implements
Join {
private final JoinType joinType;
- private final Optional<Expression> condition;
+ private final Optional<Expression> otherJoinCondition;
Review Comment:
```suggestion
private final List<Expression> nonHashJoinConjuncts;
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSlotReference.java:
##########
@@ -92,9 +92,10 @@ public List<Rule> buildRules() {
RuleType.BINDING_JOIN_SLOT.build(
logicalJoin().thenApply(ctx -> {
LogicalJoin<GroupPlan, GroupPlan> join = ctx.root;
- Optional<Expression> cond = join.getCondition()
+ Optional<Expression> cond = join.getOtherJoinCondition()
Review Comment:
this rule should bind all slot in all condition, so only bind the slot in
otherJoinCondition maybe wrong?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalHashJoin.java:
##########
@@ -38,9 +40,10 @@
RIGHT_CHILD_TYPE extends Plan>
extends AbstractPhysicalJoin<LEFT_CHILD_TYPE, RIGHT_CHILD_TYPE> {
- public PhysicalHashJoin(JoinType joinType, Optional<Expression> condition,
LogicalProperties logicalProperties,
+ public PhysicalHashJoin(JoinType joinType, List<Expression>
hashJoinConjuncts,
+ Optional<Expression> condition, LogicalProperties
logicalProperties,
Review Comment:
change `Optional<Expression> condition` to `List<Expression>
nonhashJoinConjuncts`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/logical/LogicalJoin.java:
##########
@@ -74,23 +81,42 @@ public LogicalJoin(JoinType joinType, Optional<Expression>
condition,
* @param joinType logical type for join
* @param condition on clause for join node
*/
- public LogicalJoin(JoinType joinType, Optional<Expression> condition,
+ public LogicalJoin(JoinType joinType, List<Expression> hashJoinConjuncts,
Optional<Expression> condition,
Optional<GroupExpression> groupExpression,
Optional<LogicalProperties> logicalProperties,
LEFT_CHILD_TYPE leftChild, RIGHT_CHILD_TYPE rightChild) {
super(PlanType.LOGICAL_JOIN, groupExpression, logicalProperties,
leftChild, rightChild);
this.joinType = Objects.requireNonNull(joinType, "joinType can not be
null");
- this.condition = Objects.requireNonNull(condition, "condition can not
be null");
+ this.hashJoinConjuncts = hashJoinConjuncts;
+ this.otherJoinCondition = Objects.requireNonNull(condition, "condition
can not be null");
}
- public LogicalJoin(JoinType joinType, Optional<Expression> condition,
- Optional<GroupExpression> groupExpression,
Optional<LogicalProperties> logicalProperties,
- LEFT_CHILD_TYPE leftChild, RIGHT_CHILD_TYPE rightChild,
JoinReorderContext joinReorderContext) {
- this(joinType, condition, groupExpression, logicalProperties,
leftChild, rightChild);
- this.joinReorderContext.copyFrom(joinReorderContext);
+ /**
+ * get combination of hashJoinConjuncts and condition
+ * @return combine hashJoinConjuncts and condition by AND
+ */
+ public Optional<Expression> getOtherJoinCondition() {
Review Comment:
```suggestion
public List<Expression> getNonHashJoinConjuncts() {
```
--
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
