-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(Refactor)[Nereids] Combine operator and plan #10786
Conversation
736be3c
to
6877045
Compare
public Operator getOperator() { | ||
return op; | ||
public Plan getPlan() { | ||
return plan; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to set group's logical properties to plan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the plan copyIn to Memo, the plan will be withLogicalProperties and withChildren(GroupPlan... childrens):
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
@@ -158,7 +158,8 @@ private enum DateLiteralType { | |||
DATEV2(3); | |||
|
|||
private final int value; | |||
private DateLiteralType(int value) { | |||
|
|||
DateLiteralType(int value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix checkstyle. and private enum DateLiteralType's constructor is also private, so can be omitted
|
||
@Override | ||
public Plan withLogicalProperties(Optional<LogicalProperties> logicalProperties) { | ||
return new UnboundRelation(nameParts, Optional.empty(), logicalProperties); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
groupexpression should use original one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when the plan is changed, the groupExpression must be clear to tell the memo: I create a new plan, don't skip copyIn.
This PR didn't pass the p0 test, because there are some other issue not related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
in apache#9755, we split plan into plan & operator, but in subsequent development, we found the rule became complex and counter intuition: 1. we must create an operator instance, then wrap a plan by the operator type. 2. relational algebra(operator) not contains children e.g. ```java logicalProject().then(project -> { List<NamedExpression> boundSlots = bind(project.operator.getProjects(), project.children(), project); LogicalProject op = new LogicalProject(flatBoundStar(boundSlots)); // wrap a plan return new LogicalUnaryPlan(op, project.child()); }) ``` after combine operator and plan, the code become to: ```java logicalProject().then(project -> { List<NamedExpression> boundSlots = bind(project.getProjects(), project.children(), project); return new LogicalProject(flatBoundStar(boundSlots), project.child()); }) ``` Originally, we thought it would be convenient for `Memo.copyIn()` after split plan & operator, because Memo don't known how to re-new the plan(assembling child plan in the children groups) by the plan type. So plan must provide the `withChildren()` abstract method to assembling children. The less plan type, the lower code cost we have(logical/physical with leaf/unary/binary plan, about 6 plans, no concrete plan e.g. LogicalAggregatePlan). But the convenient make negative effect that difficult to understand, and people must known the concept then can develop some new rules, and rule become ugly. So we combine the plan & operator, make the rule as simple as possible, the negative effect is we must overwrite some withXxx for all concrete plan, e.g. LogicalAggregate, PhysicalHashJoin.
Proposed changes
Issue Number: no issue
Problem Summary:
in #9755, we split plan into plan & operator, but in subsequent development, we found the rule became complex and counter intuition:
e.g.
after combine operator and plan, the code become to:
Originally, we thought it would be convenient for
Memo.copyIn()
after split plan & operator, because Memo don't known how to re-new the plan(assembling child plan in the children groups) by the plan type. So plan must provide thewithChildren()
abstract method to assembling children. The less plan type, the lower code cost we have(logical/physical with leaf/unary/binary plan, about 6 plans, no concrete plan e.g. LogicalAggregatePlan).But the convenient make negative effect that difficult to understand, and people must known the concept then can develop some new rules, and rule become ugly. So we combine the plan & operator, make the rule as simple as possible, the negative effect is we must overwrite some withXxx for all concrete plan, e.g. LogicalAggregate, PhysicalHashJoin.
Checklist(Required)