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/12 09:24:29 UTC
[GitHub] [doris] 924060929 opened a new pull request, #10786: (Refactor)[Nereids] Combine op plan
924060929 opened a new pull request, #10786:
URL: https://github.com/apache/doris/pull/10786
# Proposed changes
Issue Number: no issue
## Problem Summary:
add describe later
## Checklist(Required)
1. Does it affect the original behavior: No
2. Has unit tests been added: No Need
3. Has document been added or modified: No Need
4. Does it need to update dependencies: No
5. Are there any changes that cannot be rolled back: No
--
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 #10786: (Refactor)[Nereids] Combine operator and plan
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10786:
URL: https://github.com/apache/doris/pull/10786#discussion_r919611084
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java:
##########
@@ -158,7 +158,8 @@ private enum DateLiteralType {
DATEV2(3);
private final int value;
- private DateLiteralType(int value) {
+
+ DateLiteralType(int value) {
Review Comment:
fix checkstyle. and private enum DateLiteralType's constructor is also private, so can be omitted
--
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 #10786: (Refactor)[Nereids] Combine operator and plan
Posted by GitBox <gi...@apache.org>.
morrySnow commented on code in PR #10786:
URL: https://github.com/apache/doris/pull/10786#discussion_r918895101
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/UnboundRelation.java:
##########
@@ -72,6 +85,16 @@ public LogicalProperties computeLogicalProperties(Plan... inputs) {
return new UnboundLogicalProperties();
}
+ @Override
+ public Plan withGroupExpression(Optional<GroupExpression> groupExpression) {
+ return new UnboundRelation(nameParts, groupExpression, Optional.of(logicalProperties));
+ }
+
+ @Override
+ public Plan withLogicalProperties(Optional<LogicalProperties> logicalProperties) {
+ return new UnboundRelation(nameParts, Optional.empty(), logicalProperties);
Review Comment:
groupexpression should use original one？
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/GroupExpression.java:
##########
@@ -89,8 +89,8 @@ public void setParent(Group parent) {
this.parent = parent;
}
- public Operator getOperator() {
- return op;
+ public Plan getPlan() {
+ return plan;
Review Comment:
we need to set group's logical properties to plan
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/DateLiteral.java:
##########
@@ -158,7 +158,8 @@ private enum DateLiteralType {
DATEV2(3);
private final int value;
- private DateLiteralType(int value) {
+
+ DateLiteralType(int value) {
Review Comment:
why do this?
--
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 #10786: (Refactor)[Nereids] Combine operator and plan
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10786:
URL: https://github.com/apache/doris/pull/10786#discussion_r919612449
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/analyzer/UnboundRelation.java:
##########
@@ -72,6 +85,16 @@ public LogicalProperties computeLogicalProperties(Plan... inputs) {
return new UnboundLogicalProperties();
}
+ @Override
+ public Plan withGroupExpression(Optional<GroupExpression> groupExpression) {
+ return new UnboundRelation(nameParts, groupExpression, Optional.of(logicalProperties));
+ }
+
+ @Override
+ public Plan withLogicalProperties(Optional<LogicalProperties> logicalProperties) {
+ return new UnboundRelation(nameParts, Optional.empty(), logicalProperties);
Review Comment:
when the plan is changed, the groupExpression must be clear to tell the memo: I create a new plan, don't skip copyIn.
--
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] morningman commented on pull request #10786: (Refactor)[Nereids] Combine operator and plan
Posted by GitBox <gi...@apache.org>.
morningman commented on PR #10786:
URL: https://github.com/apache/doris/pull/10786#issuecomment-1183003757
This PR didn't pass the p0 test, because there are some other issue not related to this PR.
So I skip p0 for this PR.
--
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] EmmyMiao87 merged pull request #10786: (Refactor)[Nereids] Combine operator and plan
Posted by GitBox <gi...@apache.org>.
EmmyMiao87 merged PR #10786:
URL: https://github.com/apache/doris/pull/10786
--
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 #10786: (Refactor)[Nereids] Combine operator and plan
Posted by GitBox <gi...@apache.org>.
924060929 commented on code in PR #10786:
URL: https://github.com/apache/doris/pull/10786#discussion_r919614263
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/memo/GroupExpression.java:
##########
@@ -89,8 +89,8 @@ public void setParent(Group parent) {
this.parent = parent;
}
- public Operator getOperator() {
- return op;
+ public Plan getPlan() {
+ return plan;
Review Comment:
when the plan copyIn to Memo, the plan will be withLogicalProperties and withChildren(GroupPlan... childrens):
```java
private Plan replaceChildrenToGroupPlan(Plan plan, List<Group> childrenGroups) {
List<Plan> groupPlanChildren = childrenGroups.stream()
.map(group -> new GroupPlan(group))
.collect(ImmutableList.toImmutableList());
LogicalProperties logicalProperties = plan.getLogicalProperties();
return plan.withChildren(groupPlanChildren)
.withLogicalProperties(Optional.of(logicalProperties));
}
```
so the plan in the memo contains the logicalProperties same as the logicalProperties in the groupExpression, and replace GroupPlan as children
--
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