planner: fix wrong agg function when agg push down union #17022
Conversation
lzmhhh123
added
type/bugfix
ghost
requested review from
Codecov Report
@@ Coverage Diff @@
## master #17022 +/- ##
================================================
+ Coverage 79.8021% 79.8070% +0.0048%
================================================
Files 520 520
Lines 139733 139623 -110
================================================
- Hits 111510 111429 -81
+ Misses 19252 19226 -26
+ Partials 8971 8968 -3 |
| " └─Union_12 16000.00 root ", | ||
| " ├─HashAgg_16 8000.00 root group by:test.pt.b, funcs:firstrow(test.pt.b)->test.pt.b", | ||
| " ├─HashAgg_16 8000.00 root group by:test.pt.b, funcs:firstrow(test.pt.b)->test.pt.b, funcs:firstrow(test.pt.b)->test.pt.b", |
There was a problem hiding this comment.
The firstrow is duplicated here?
There was a problem hiding this comment.
Yes. For correctness.
There was a problem hiding this comment.
Can we check if the firstrow is already existed?
BTW, I'm confused why we need to add firstrow here, it is supposed be added by planbuilder？
There was a problem hiding this comment.
Because the partial aggregation should return the group by columns. In that way, final aggregation could execute.
XuHuaiyu
added
needs-cherry-pick-3.0
priority/release-blocker
| PARTITION p3 VALUES LESS THAN (40), | ||
| PARTITION p4 VALUES LESS THAN (MAXVALUE) | ||
| )`) | ||
| tk.MustExec("insert into t1 values (0, 0), (1, 1), (1, 2), (1, 3), (2, 4), (2, 5), (2, 6), (3, 7), (3, 10), (3, 11), (12, 12), (12, 13), (14, 14), (14, 15), (20, 20), (20, 21), (20, 22), (23, 23), (23, 24), (23, 25), (31, 30), (31, 31), (31, 32), (33, 33), (33, 34), (33, 35), (36, 36), (80, 80), (90, 90), (100, 100)") |
There was a problem hiding this comment.
set @@tidb_opt_agg_push_down=1
| @@ -340,6 +340,11 @@ func (a *aggregationPushDownSolver) pushAggCrossUnion(agg *LogicalAggregation, u | |||
| for _, gbyExpr := range agg.GroupByItems { | |||
| newExpr := expression.ColumnSubstitute(gbyExpr, unionSchema, expression.Column2Exprs(unionChild.Schema().Columns)) | |||
| newAgg.GroupByItems = append(newAgg.GroupByItems, newExpr) | |||
| firstRow, err := aggregation.NewAggFuncDesc(agg.ctx, ast.AggFuncFirstRow, []expression.Expression{gbyExpr}, false) | |||
There was a problem hiding this comment.
Check if first_row(gbyExpr) already exists before build a new one?
There was a problem hiding this comment.
If we do this, we need to change the schema for partial aggregation. I'm afraid there would be some other corner cases, so I prefer to keep the duplicated first_row.
|
/merge |
sre-bot
added
the
status/can-merge
|
/run-all-tests |
Signed-off-by: sre-bot <sre-bot@pingcap.com>
|
cherry pick to release-3.0 in PR #17326 |
Signed-off-by: sre-bot <sre-bot@pingcap.com>
|
cherry pick to release-3.1 in PR #17327 |
|
cherry pick to release-4.0 in PR #17328 |
What problem does this PR solve?
Issue Number: close #16861
Problem Summary: when the aggregation is pushed down the union, we should append the first row for group by items.
What is changed and how it works?
What's Changed: append the first row when the partial aggregation is built under the union.
Related changes
Check List
Tests
Side effects
Release note
Fix the panic when aggregation push down enable on the partition table.