-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
plan: support pushing agg past union all. #2150
Conversation
return agg | ||
} | ||
|
||
func (a *aggPushDownSolver) pushAggCrossUnion(agg *Aggregation, oldSchema expression.Schema, p LogicalPlan) LogicalPlan { |
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.
oldSchema
-> unionSchema
p
-> unionChild
} else if union, ok1 := child.(*Union); ok1 { | ||
pushedAgg := a.makeNewAgg(agg.AggFuncs, agg.groupByCols) | ||
oldSchema := union.schema | ||
union.SetSchema(pushedAgg.schema) |
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.
Maybe update union schema after push aggregate to children is better?
So we don't to define oldSchema
.
LGTM |
unionChild.SetParents(newAgg) | ||
return newAgg | ||
} | ||
|
||
// aggPushDown tries to push down aggregate functions to join paths. | ||
func (a *aggPushDownSolver) aggPushDown(p LogicalPlan) { | ||
if agg, ok := p.(*Aggregation); ok { |
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.
It's better to write as follows:
agg, ok := p.(*Aggregation);
if !ok {return}
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 shouldn't return , instead, we should go to line 342
tk.MustExec("create table t3 (c int, d int)") | ||
tk.MustExec("insert t3 values (3, 2)") | ||
tk.MustExec("insert t3 values (4, 3)") | ||
|
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.
Remove this empty line.
LGTM |
support pushing agg past union all. This algo is very simple.
@coocood @zimulala @shenli @winoros @XuHuaiyu PTAL