Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
planner: implement aggregation eliminate optimize trace #30114
planner: implement aggregation eliminate optimize trace #30114
Changes from 13 commits
c7b2f0b
f982f3e
d371906
72d33bd
780516f
4be62fa
4c519a1
ba93891
2d61e84
a4bc45a
9080549
790df35
dc6d42d
cd6ec77
518c657
713d65c
89df62e
105fdb3
cdf1db4
d40ea55
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Are we going to have different implementations for different logical plans? If not, can we align them to one func?
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 attach explain info in trace, thus it should be implemented by each logical operator.
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.
ExplainInfo is also defined as an interface, what about "func buildLogicalPlanTrace(p *baseLogicalPlan) {...}"
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.
updated.
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.
I tend to move all the optimize trace test in a dedicated file.
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 you check null here again, since you've already wrap tracer to logicalOptimizeOp and do check in logicalOptimizeOp? You can simplify here. Since wrap, rule should not see and call tracer directly.
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.
And after simplification, you do not need the 2 funcs here, just call appendStep... in tryToEliminate.
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.
updated, I tend to wrap
appendStep
in an extra function due to sometimes constructingreason
andaction
will be complex, thus I separate it from the main logic.