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/20 10:15:35 UTC
[GitHub] [doris] englefly opened a new pull request, #11043: [feature] (Nereids) Merge memo group recursively
englefly opened a new pull request, #11043:
URL: https://github.com/apache/doris/pull/11043
# Proposed changes
Issue Number: close #xxx
## Problem Summary:
Describe the overview of changes.
## Checklist(Required)
1. Does it affect the original behavior: (Yes/No/I Don't know)
2. Has unit tests been added: (Yes/No/No Need)
3. Has document been added or modified: (Yes/No/No Need)
4. Does it need to update dependencies: (Yes/No)
5. Are there any changes that cannot be rolled back: (Yes/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] 924060929 commented on a diff in pull request #11043: [feature] (Nereids) Merge memo group recursively
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #11043:
URL: https://github.com/apache/doris/pull/11043#discussion_r926376161
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -188,19 +189,24 @@ private void mergeGroup(Group source, Group destination) {
children.set(i, destination);
}
}
- if (groupExpressions.containsKey(groupExpression)) {
- // TODO: need to merge group recursively
- groupExpression.getOwnerGroup().removeGroupExpression(groupExpression);
+ GroupExpression that = groupExpressions.get(groupExpression);
+ if (that != null) {
+ mergeGroup(groupExpression.getOwnerGroup(), that.getOwnerGroup());
} else {
groupExpressions.put(groupExpression, groupExpression);
}
}
- for (GroupExpression groupExpression : source.getLogicalExpressions()) {
- source.removeGroupExpression(groupExpression);
+
+ Iterator<GroupExpression> iterator = source.getLogicalExpressions().iterator();
+ while (iterator.hasNext()) {
+ GroupExpression groupExpression = iterator.next();
+ iterator.remove();
destination.addGroupExpression(groupExpression);
}
- for (GroupExpression groupExpression : source.getPhysicalExpressions()) {
- source.removeGroupExpression(groupExpression);
+ iterator = source.getPhysicalExpressions().iterator();
Review Comment:
```suggestion
if (source != destination) {
// copy sourceGroup's physicalExpressions to destination group
destination.addPhysicalExpressions(source.getPhysicalExpressions());
source.clearPhysicalExpressions();
}
```
--
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 #11043: [feature] (Nereids) Merge memo group recursively
Posted by GitBox <gi...@apache.org>.
924060929 merged PR #11043:
URL: https://github.com/apache/doris/pull/11043
--
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 #11043: [feature] (Nereids) Merge memo group recursively
Posted by GitBox <gi...@apache.org>.
englefly commented on code in PR #11043:
URL: https://github.com/apache/doris/pull/11043#discussion_r927225634
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Memo.java:
##########
@@ -188,19 +189,24 @@ private void mergeGroup(Group source, Group destination) {
children.set(i, destination);
}
}
- if (groupExpressions.containsKey(groupExpression)) {
- // TODO: need to merge group recursively
- groupExpression.getOwnerGroup().removeGroupExpression(groupExpression);
+ GroupExpression that = groupExpressions.get(groupExpression);
+ if (that != null) {
+ mergeGroup(groupExpression.getOwnerGroup(), that.getOwnerGroup());
} else {
groupExpressions.put(groupExpression, groupExpression);
}
}
- for (GroupExpression groupExpression : source.getLogicalExpressions()) {
- source.removeGroupExpression(groupExpression);
+
+ Iterator<GroupExpression> iterator = source.getLogicalExpressions().iterator();
+ while (iterator.hasNext()) {
+ GroupExpression groupExpression = iterator.next();
+ iterator.remove();
destination.addGroupExpression(groupExpression);
}
- for (GroupExpression groupExpression : source.getPhysicalExpressions()) {
- source.removeGroupExpression(groupExpression);
+ iterator = source.getPhysicalExpressions().iterator();
Review Comment:
add two API in Group
public void moveLogicalExpressionOwnership(Group target);
public void movePhysicalExpressionOwnership(Group 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] morrySnow commented on a diff in pull request #11043: [feature] (Nereids) Merge memo group recursively
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #11043:
URL: https://github.com/apache/doris/pull/11043#discussion_r925861054
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/memo/MemoTest.java:
##########
@@ -56,4 +57,26 @@ public void testCopyIn() {
rootGroup.logicalExpressionsAt(0).child(0).logicalExpressionsAt(0).child(0).logicalExpressionsAt(0)
.getPlan().getType());
}
+
+ @Test
+ public void testMergeGroup() {
+
+ UnboundRelation rel1 = new UnboundRelation(Lists.newArrayList("test"));
+ LogicalProject proj1 = new LogicalProject(
+ ImmutableList.of(new SlotReference(new ExprId(1), "name", StringType.INSTANCE, true, ImmutableList.of("test"))),
+ rel1
+ );
+
+ Memo memo = new Memo(proj1);
+
+ UnboundRelation rel2 = new UnboundRelation(Lists.newArrayList("test2"));
+ LogicalProject proj2 = new LogicalProject(
+ ImmutableList.of(new SlotReference(new ExprId(1), "name", StringType.INSTANCE, true, ImmutableList.of("test"))),
+ rel2
+ );
+ memo.copyIn(proj2, null, false);
+
+ memo.copyIn(rel1, memo.getGroups().get(2), true);
+ Assert.assertEquals(2, memo.getGroups().size());
Review Comment:
should check all GroupExpression's owner group.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/memo/MemoTest.java:
##########
@@ -56,4 +57,26 @@ public void testCopyIn() {
rootGroup.logicalExpressionsAt(0).child(0).logicalExpressionsAt(0).child(0).logicalExpressionsAt(0)
.getPlan().getType());
}
+
+ @Test
+ public void testMergeGroup() {
+
+ UnboundRelation rel1 = new UnboundRelation(Lists.newArrayList("test"));
Review Comment:
we prefer whole word variable name
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/Group.java:
##########
@@ -113,6 +114,23 @@ public GroupExpression removeGroupExpression(GroupExpression groupExpression) {
return groupExpression;
}
+ /**
+ * remove all logical and physical expressions
+ * if this group is merged with other group, all the expressions should be removed.
+ */
+ public void removeAllExpressions() {
+ Iterator<GroupExpression> iter = logicalExpressions.iterator();
Review Comment:
i think we should not setOwnerGroup to null here. since in java all things is reference, we do add to destination group first and then to remove all expressions in source. if we do setOwnerGroup to null here, the result will be wrong
BTW, maybe call list.clear is better.
--
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