924060929 commented on code in PR #13915:
URL: https://github.com/apache/doris/pull/13915#discussion_r1011790554
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/GroupExpression.java:
##########
@@ -106,50 +130,47 @@ public List<Group> children() {
return children;
}
- public void setChildren(ImmutableList<Group> children) {
- this.children.forEach(g -> g.removeParentExpression(this));
- this.children = children;
- this.children.forEach(g -> g.addParentExpression(this));
- }
-
/**
- * replaceChild.
+ * use newChild to replace oldChild in Children of this GroupExpr.
+ * We replace something:
+ * - Memo: groupExpressions
+ * - Child Group: parentGroupExpr
+ * - Parent Group:
+ * - - children: logical|physical
+ * - - lowestCostPlans
+ * - Plan: groupExpression
*
- * @param originChild origin child group
+ * @param oldChild origin child group
* @param newChild new child group
*/
- public void replaceChild(Group originChild, Group newChild) {
- originChild.removeParentExpression(this);
- List<Group> groups =
Lists.newArrayListWithCapacity(this.children.size());
- for (int i = 0; i < children.size(); i++) {
- if (children.get(i) == originChild) {
- groups.add(newChild);
- } else {
- groups.add(child(i));
- }
+ public GroupExpression replaceChild(Map<GroupExpression, GroupExpression>
groupExpressions,
+ Group oldChild, Group newChild) {
+ GroupExpression newGroupExpression =
withChildren(Utils.replaceListWithNew(children, oldChild, newChild));
+
+ // replace childrenGroup {parent}
+ newGroupExpression.children().forEach(childGroup ->
+ Utils.replaceList(childGroup.getParentGroupExpressions(),
this, newGroupExpression));
+
+ // replace ownerGroup {children} (logical|physical, lowestCostPlans)
+ // TODO: maybe no need replace physical.
+ newGroupExpression.getOwnerGroup().replaceGroupExpression(this,
newGroupExpression);
+
+ // replace Memo {groupExpressions}
+ groupExpressions.remove(this);
+ groupExpressions.put(newGroupExpression, newGroupExpression);
+
+ // replace Plan {GroupExpr}
+ if (plan.getGroupExpression().isPresent()) {
+ this.plan.setGroupExpression(newGroupExpression);
Review Comment:
plan should be immutable
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/AbstractPlan.java:
##########
@@ -77,6 +77,10 @@ public Optional<GroupExpression> getGroupExpression() {
return groupExpression;
}
+ public void setGroupExpression(GroupExpression groupExpression) {
+ this.groupExpression = Optional.of(groupExpression);
+ }
+
Review Comment:
plan should be immutable so can not support setXxx. You can invoke
`withGroupExpression()`
--
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
