wangshuo128 commented on code in PR #11502: URL: https://github.com/apache/doris/pull/11502#discussion_r938494772
########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/LogicalJoinToNestedLoopJoin.java: ########## @@ -0,0 +1,40 @@ +// 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.implementation; + +import org.apache.doris.nereids.rules.Rule; +import org.apache.doris.nereids.rules.RuleType; +import org.apache.doris.nereids.trees.plans.physical.PhysicalNestedLoopJoin; + +/** + * Implementation rule that convert logical join to physical nested loop join. + */ +public class LogicalJoinToNestedLoopJoin extends OneImplementationRuleFactory { + @Override + public Rule build() { + return logicalJoin() + .when(join -> !join.getCondition().isPresent()) Review Comment: To keep consistent with `PhysicalPlanTranlator`, I suggest to use `joinType.equals(JoinType.CROSS_JOIN) || (joinType.equals(JoinType.INNER_JOIN) && !nestedLoopJoin.getCondition().isPresent())` and extract this as a method to `JoinUtils`, such as `shouldNestedLoooJoin`? ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/physical/PhysicalNestedLoopJoin.java: ########## @@ -0,0 +1,91 @@ +// 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.trees.plans.physical; + +import org.apache.doris.nereids.memo.GroupExpression; +import org.apache.doris.nereids.properties.LogicalProperties; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.plans.JoinType; +import org.apache.doris.nereids.trees.plans.Plan; +import org.apache.doris.nereids.trees.plans.PlanType; +import org.apache.doris.nereids.trees.plans.visitor.PlanVisitor; + +import com.google.common.base.Preconditions; + +import java.util.List; +import java.util.Optional; + +/** + * Use nested loop algorithm to do join. + */ +public class PhysicalNestedLoopJoin< + LEFT_CHILD_TYPE extends Plan, + RIGHT_CHILD_TYPE extends Plan> + extends PhysicalJoin<LEFT_CHILD_TYPE, RIGHT_CHILD_TYPE> { + + public PhysicalNestedLoopJoin(JoinType joinType, Optional<Expression> condition, + LogicalProperties logicalProperties, LEFT_CHILD_TYPE leftChild, RIGHT_CHILD_TYPE rightChild) { + this(joinType, condition, Optional.empty(), logicalProperties, leftChild, rightChild); + } + + /** + * Constructor of PhysicalNestedLoopJoin. + * + * @param joinType Which join type, left semi join, inner join... + * @param condition join condition. + */ + public PhysicalNestedLoopJoin(JoinType joinType, Optional<Expression> condition, + Optional<GroupExpression> groupExpression, LogicalProperties logicalProperties, + LEFT_CHILD_TYPE leftChild, RIGHT_CHILD_TYPE rightChild) { + super(PlanType.PHYSICAL_NESTED_LOOP_JOIN, joinType, condition, + groupExpression, logicalProperties, leftChild, rightChild); + } + + @Override + public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { + return visitor.visitPhysicalNestedLoopJoin((PhysicalNestedLoopJoin<Plan, Plan>) this, context); + } + + @Override + public String toString() { Review Comment: Maybe we could pull up this to the abstract class in the future. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/stats/JoinEstimation.java: ########## @@ -41,10 +43,11 @@ public class JoinEstimation { * Do estimate. */ public static StatsDeriveResult estimate(StatsDeriveResult leftStats, StatsDeriveResult rightStats, Review Comment: Maybe passing a whole join node as the parameter is more clear, rather than properties in join. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java: ########## @@ -99,6 +100,40 @@ public PhysicalProperties visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> has return leftOutputProperty; } + @Override + public PhysicalProperties visitPhysicalNestedLoopJoin(PhysicalNestedLoopJoin<Plan, Plan> nestedLoopJoin, Review Comment: I didn't go through the logic carefully. IIUC, the property deriver framework and logic is under development and not ready to be used. Maybe someone who is expertise in this should take a look. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java: ########## @@ -373,6 +375,30 @@ public PlanFragment visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> hashJoin, } } + @Override + public PlanFragment visitPhysicalNestedLoopJoin(PhysicalNestedLoopJoin<Plan, Plan> nestedLoopJoin, + PlanTranslatorContext context) { + // NOTICE: We must visit from right to left, to ensure the last fragment is root fragment + PlanFragment rightFragment = nestedLoopJoin.child(1).accept(this, context); + PlanFragment leftFragment = nestedLoopJoin.child(0).accept(this, context); + PlanNode leftFragmentPlanRoot = leftFragment.getPlanRoot(); + PlanNode rightFragmentPlanRoot = rightFragment.getPlanRoot(); + JoinType joinType = nestedLoopJoin.getJoinType(); + + if (joinType.equals(JoinType.CROSS_JOIN) Review Comment: Please refer to the review comments on rule predicate of `LogicalJoinToNestedLoopJoin`. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java: ########## @@ -373,6 +375,30 @@ public PlanFragment visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> hashJoin, } } + @Override + public PlanFragment visitPhysicalNestedLoopJoin(PhysicalNestedLoopJoin<Plan, Plan> nestedLoopJoin, + PlanTranslatorContext context) { + // NOTICE: We must visit from right to left, to ensure the last fragment is root fragment Review Comment: Should we add a helper method to wrap this logic? Maybe something like `private List<PlanFragment>` postOrderVisitChildren(PhysicalPlan plan, PlanVisitor visitor, Context context)`. Could add a todo and refactor this in the next PR. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/properties/RequestPropertyDeriver.java: ########## @@ -92,6 +93,31 @@ public Void visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> hashJoin, PlanCon return null; } + @Override + public Void visitPhysicalNestedLoopJoin(PhysicalNestedLoopJoin<Plan, Plan> nestedLoopJoin, PlanContext context) { Review Comment: ditto. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/cost/CostCalculator.java: ########## @@ -133,5 +134,32 @@ public CostEstimate visitPhysicalHashJoin(PhysicalHashJoin<Plan, Plan> physicalH 0); } + @Override + public CostEstimate visitPhysicalNestedLoopJoin(PhysicalNestedLoopJoin<Plan, Plan> nestedLoopJoin, Review Comment: Similar to `ChildOutputPropertyDeriver`, maybe we should revisit this logic when tuning join logic in cascades. -- 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
