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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
