[GitHub] [doris] 924060929 commented on a diff in pull request #11342: [refactor](Nereids)split rewrite and insert into memo to 2 functions

[GitBox <gi...@apache.org>]

924060929 commented on code in PR #11342:
URL: https://github.com/apache/doris/pull/11342#discussion_r933267228


##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -122,44 +124,63 @@ private Plan groupToTreeNode(Group group) {
     }
 
     /**
-     * Insert or rewrite groupExpression to target group.
+     * Insert groupExpression to target group.
      * If group expression is already in memo and target group is not null, we merge two groups.
      * If target is null, generate new group.
-     * If rewrite is true, rewrite the groupExpression to target group.
+     * If target is not null, add group expression to target group
      *
      * @param groupExpression groupExpression to insert
-     * @param target target group to insert or rewrite groupExpression
-     * @param rewrite whether to rewrite the groupExpression to target group
+     * @param target target group to insert groupExpression
      * @return a pair, in which the first element is true if a newly generated groupExpression added into memo,
      *         and the second element is a reference of node in Memo
      */
-    private Pair<Boolean, GroupExpression> insertOrRewriteGroupExpression(GroupExpression groupExpression, Group target,
-            boolean rewrite, LogicalProperties logicalProperties) {
+    private Pair<Boolean, GroupExpression> insertGroupExpression(
+            GroupExpression groupExpression, Group target, LogicalProperties logicalProperties) {
         GroupExpression existedGroupExpression = groupExpressions.get(groupExpression);
         if (existedGroupExpression != null) {
-            Group mergedGroup = existedGroupExpression.getOwnerGroup();
             if (target != null && !target.getGroupId().equals(existedGroupExpression.getOwnerGroup().getGroupId())) {
-                mergedGroup = mergeGroup(target, existedGroupExpression.getOwnerGroup());
-            }
-            if (rewrite) {
-                mergedGroup.setLogicalProperties(logicalProperties);
+                mergeGroup(target, existedGroupExpression.getOwnerGroup());
             }
-            return new Pair(false, existedGroupExpression);
+            return new Pair<>(false, existedGroupExpression);
         }
         if (target != null) {
-            if (rewrite) {
-                GroupExpression oldExpression = target.rewriteLogicalExpression(groupExpression, logicalProperties);
-                groupExpressions.remove(oldExpression);
-            } else {
-                target.addGroupExpression(groupExpression);
-            }
+            target.addGroupExpression(groupExpression);
+        } else {
+            Group group = new Group(groupIdGenerator.getNextId(), groupExpression, logicalProperties);
+            groups.add(group);
+        }
+        groupExpressions.put(groupExpression, groupExpression);
+        return new Pair<>(true, groupExpression);
+    }
+
+    /**
+     * Rewrite groupExpression to target group.
+     * If group expression is already in memo and target group is not null, we replace logical properties.

Review Comment:
   this comment inconsistent with the code. the code is 'If group expression is already in memo, we replace logical properties regardless the target group present or not', and we can add the comment: 'for replace UnboundLogicalProperties to LogicalProperties' 
   
   



-- 
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-unsubscribe@doris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org