englefly commented on code in PR #11342:
URL: https://github.com/apache/doris/pull/11342#discussion_r933254780
##########
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());
Review Comment:
After mergeGroup(), the first argument `target` may be removed from memo.
`target` is input parameter, the caller may use it after `copyIn()`.
How about `mergeGroup(existedGroupExpression.getOwnerGroup(), target);` ?
--
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
