You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2022/07/29 11:29:53 UTC
[GitHub] [doris] morrySnow opened a new pull request, #11342: [refactor](Nereids)split rewrite and insert into memo to 2 functions
morrySnow opened a new pull request, #11342:
URL: https://github.com/apache/doris/pull/11342
## Problem Summary:
Split rewrite and insert into memo to 2 functions to make the code easy to read.
## Checklist(Required)
1. Does it affect the original behavior:
- [ ] Yes
- [x] No
- [ ] I don't know
2. Has unit tests been added:
- [x] Yes
- [ ] No
- [ ] No Need
3. Has document been added or modified:
- [ ] Yes
- [x] No
- [ ] No Need
4. Does it need to update dependencies:
- [ ] Yes
- [x] No
5. Are there any changes that cannot be rolled back:
- [ ] Yes
- [x] No
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc...
--
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
[GitHub] [doris] github-actions[bot] commented on pull request #11342: [refactor](Nereids)split rewrite and insert into memo to 2 functions
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #11342:
URL: https://github.com/apache/doris/pull/11342#issuecomment-1200755964
PR approved by anyone and no changes requested.
--
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
[GitHub] [doris] 924060929 commented on a diff in pull request #11342: [refactor](Nereids)split rewrite and insert into memo to 2 functions
Posted by 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
[GitHub] [doris] englefly commented on a diff in pull request #11342: [refactor](Nereids)split rewrite and insert into memo to 2 functions
Posted by GitBox <gi...@apache.org>.
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-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
[GitHub] [doris] 924060929 merged pull request #11342: [refactor](Nereids)split rewrite and insert into memo to 2 functions
Posted by GitBox <gi...@apache.org>.
924060929 merged PR #11342:
URL: https://github.com/apache/doris/pull/11342
--
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
[GitHub] [doris] morrySnow commented on pull request #11342: [refactor](Nereids)split rewrite and insert into memo to 2 functions
Posted by GitBox <gi...@apache.org>.
morrySnow commented on PR #11342:
URL: https://github.com/apache/doris/pull/11342#issuecomment-1199175298
@924060929 @englefly PTAL
--
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