From 4424874237f2be5d26deceae4f49ba4011360a80 Mon Sep 17 00:00:00 2001 From: jakevin Date: Wed, 11 Jan 2023 15:47:37 +0800 Subject: [PATCH] [fix](Nereids): move parentExpression in moveOwnership() (#15786) --- .../org/apache/doris/nereids/memo/Group.java | 45 ++++++------------- .../org/apache/doris/nereids/memo/Memo.java | 17 +++---- .../org/apache/doris/nereids/util/Utils.java | 2 +- 3 files changed, 19 insertions(+), 45 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Group.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Group.java index 80244a1ca8..0e90022556 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Group.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Group.java @@ -46,7 +46,7 @@ import java.util.stream.Collectors; */ public class Group { private final GroupId groupId; - // Save all parent GroupExpression to avoid travsing whole Memo. + // Save all parent GroupExpression to avoid traversing whole Memo. private final IdentityHashMap parentExpressions = new IdentityHashMap<>(); private final List logicalExpressions = Lists.newArrayList(); @@ -261,47 +261,28 @@ public class Group { } /** - * move the ownerGroup of all logical expressions to target group + * move the ownerGroup to target group * if this.equals(target), do nothing. * * @param target the new owner group of expressions */ - public void moveLogicalExpressionOwnership(Group target) { + public void moveOwnership(Group target) { if (equals(target)) { return; } - for (GroupExpression expression : logicalExpressions) { - target.addGroupExpression(expression); - } + + // move parentExpressions Ownership + parentExpressions.keySet().forEach(kv -> target.addParentExpression(kv)); + parentExpressions.clear(); + + // move LogicalExpression PhysicalExpression Ownership + logicalExpressions.forEach(expr -> target.addGroupExpression(expr)); logicalExpressions.clear(); - } - - /** - * move the ownerGroup of all physical expressions to target group - * if this.equals(target), do nothing. - * - * @param target the new owner group of expressions - */ - public void movePhysicalExpressionOwnership(Group target) { - if (equals(target)) { - return; - } - for (GroupExpression expression : physicalExpressions) { - target.addGroupExpression(expression); - } + // movePhysicalExpressionOwnership + physicalExpressions.forEach(expr -> target.addGroupExpression(expr)); physicalExpressions.clear(); - } - /** - * move the ownerGroup of all lowestCostPlans to target group - * if this.equals(target), do nothing. - * - * @param target the new owner group of expressions - */ - public void moveLowestCostPlansOwnership(Group target) { - if (equals(target)) { - return; - } + // moveLowestCostPlansOwnership lowestCostPlans.forEach((physicalProperties, costAndGroupExpr) -> { GroupExpression bestGroupExpression = costAndGroupExpr.second; // change into target group. diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java index 6ca12867fc..0de2cfce72 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java @@ -33,6 +33,7 @@ import org.apache.doris.nereids.trees.plans.Plan; import org.apache.doris.nereids.trees.plans.logical.LogicalOlapScan; import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; import org.apache.doris.nereids.trees.plans.logical.LogicalProject; +import org.apache.doris.nereids.util.Utils; import org.apache.doris.qe.ConnectContext; import org.apache.doris.statistics.StatsDeriveResult; @@ -131,12 +132,11 @@ public class Memo { return copyOutAll(root); } - public List copyOutAll(Group group) { + private List copyOutAll(Group group) { List logicalExpressions = group.getLogicalExpressions(); - List plans = logicalExpressions.stream() + return logicalExpressions.stream() .flatMap(groupExpr -> copyOutAll(groupExpr).stream()) .collect(Collectors.toList()); - return plans; } private List copyOutAll(GroupExpression logicalExpression) { @@ -446,12 +446,7 @@ public class Memo { // After change GroupExpression children, the hashcode will change, // so need to reinsert into map. groupExpressions.remove(groupExpression); - List children = groupExpression.children(); - for (int i = 0; i < children.size(); i++) { - if (children.get(i).equals(source)) { - children.set(i, destination); - } - } + Utils.replaceList(groupExpression.children(), source, destination); GroupExpression that = groupExpressions.get(groupExpression); if (that != null && that.getOwnerGroup() != null @@ -467,9 +462,7 @@ public class Memo { } if (!source.equals(destination)) { // TODO: stats and other - source.moveLogicalExpressionOwnership(destination); - source.movePhysicalExpressionOwnership(destination); - source.moveLowestCostPlansOwnership(destination); + source.moveOwnership(destination); groups.remove(source.getGroupId()); } return destination; diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/util/Utils.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/util/Utils.java index a48aac5d2f..c5d8e5eaac 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/util/Utils.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/util/Utils.java @@ -212,7 +212,7 @@ public class Utils { public static void replaceList(List list, T oldItem, T newItem) { for (int i = 0; i < list.size(); i++) { - if (list.get(i) == oldItem) { + if (list.get(i).equals(oldItem)) { list.set(i, newItem); } }