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 17:16:19 UTC
[GitHub] [doris] morrySnow commented on a diff in pull request #11043: [feature] (Nereids) Merge memo group recursively
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