From 62766de25a28bf35f8f15aeae83488eb0b445335 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Mon, 30 Oct 2023 18:26:04 +0800 Subject: [PATCH] planner: fix group_concat function couldn't resolve the index of its order-by item (#46419) (#47120) close pingcap/tidb#41986 --- expression/integration_test.go | 11 +++++++ planner/core/rule_aggregation_push_down.go | 35 ++++++++++++++++++++++ planner/core/rule_eliminate_projection.go | 3 ++ 3 files changed, 49 insertions(+) diff --git a/expression/integration_test.go b/expression/integration_test.go index e16210a1e7bc3..5f4028fd6ffe2 100644 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -7962,3 +7962,14 @@ func TestIssue45410(t *testing.T) { tk.MustExec("INSERT INTO t1 VALUES (0);") tk.MustQuery("SELECT c1>=CAST('-787360724' AS TIME) FROM t1;").Check(testkit.Rows("1")) } + +func TestIssue41986(t *testing.T) { + store := testkit.CreateMockStore(t) + tk := testkit.NewTestKit(t, store) + + tk.MustExec("use test") + tk.MustExec("CREATE TABLE poi_clearing_time_topic (effective_date datetime DEFAULT NULL , clearing_time int(11) DEFAULT NULL);") + tk.MustExec("insert into poi_clearing_time_topic values ('2023:08:25', 1)") + // shouldn't report they can't find column error and return the right result. + tk.MustQuery("SELECT GROUP_CONCAT(effective_date order by stlmnt_hour DESC) FROM ( SELECT (COALESCE(pct.clearing_time, 0)/3600000) AS stlmnt_hour ,COALESCE(pct.effective_date, '1970-01-01 08:00:00') AS effective_date FROM poi_clearing_time_topic pct ORDER BY pct.effective_date DESC ) a;").Check(testkit.Rows("2023-08-25 00:00:00")) +} diff --git a/planner/core/rule_aggregation_push_down.go b/planner/core/rule_aggregation_push_down.go index 81a99b2bb7e29..a1ac4de020b3c 100644 --- a/planner/core/rule_aggregation_push_down.go +++ b/planner/core/rule_aggregation_push_down.go @@ -23,6 +23,7 @@ import ( "github.com/pingcap/tidb/expression/aggregation" "github.com/pingcap/tidb/parser/ast" "github.com/pingcap/tidb/parser/mysql" + "github.com/pingcap/tidb/planner/util" "github.com/pingcap/tidb/sessionctx" "github.com/pingcap/tidb/types" ) @@ -553,6 +554,8 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim } oldAggFuncsArgs := make([][]expression.Expression, 0, len(agg.AggFuncs)) newAggFuncsArgs := make([][]expression.Expression, 0, len(agg.AggFuncs)) + oldAggOrderItems := make([][]*util.ByItems, 0, len(agg.AggFuncs)) + newAggOrderItems := make([][]*util.ByItems, 0, len(agg.AggFuncs)) if noSideEffects { for _, aggFunc := range agg.AggFuncs { oldAggFuncsArgs = append(oldAggFuncsArgs, aggFunc.Args) @@ -565,6 +568,27 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim break } newAggFuncsArgs = append(newAggFuncsArgs, newArgs) + // for ordeByItems, treat it like agg func's args, if it can be substituted by underlying projection's expression recording them temporarily. + if len(aggFunc.OrderByItems) != 0 { + oldAggOrderItems = append(oldAggOrderItems, aggFunc.OrderByItems) + newOrderByItems := make([]expression.Expression, 0, len(aggFunc.OrderByItems)) + for _, oby := range aggFunc.OrderByItems { + newOrderByItems = append(newOrderByItems, expression.ColumnSubstitute(oby.Expr, proj.schema, proj.Exprs)) + } + if ExprsHasSideEffects(newOrderByItems) { + noSideEffects = false + break + } + oneAggOrderByItems := make([]*util.ByItems, 0, len(aggFunc.OrderByItems)) + for i, obyExpr := range newOrderByItems { + oneAggOrderByItems = append(oneAggOrderByItems, &util.ByItems{Expr: obyExpr, Desc: aggFunc.OrderByItems[i].Desc}) + } + newAggOrderItems = append(newAggOrderItems, oneAggOrderByItems) + } else { + // occupy the pos for convenience of subscript index + oldAggOrderItems = append(oldAggOrderItems, nil) + newAggOrderItems = append(newAggOrderItems, nil) + } } } for i, funcsArgs := range oldAggFuncsArgs { @@ -574,6 +598,16 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim break } } + for j, item := range newAggOrderItems { + if item == nil { + continue + } + // substitution happened, check the eval type compatibility. + if oldAggOrderItems[i][j].Expr.GetType().EvalType() != newAggOrderItems[i][j].Expr.GetType().EvalType() { + noSideEffects = false + break + } + } if !noSideEffects { break } @@ -582,6 +616,7 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim agg.GroupByItems = newGbyItems for i, aggFunc := range agg.AggFuncs { aggFunc.Args = newAggFuncsArgs[i] + aggFunc.OrderByItems = newAggOrderItems[i] } projChild := proj.children[0] agg.SetChildren(projChild) diff --git a/planner/core/rule_eliminate_projection.go b/planner/core/rule_eliminate_projection.go index 1459896f30b01..a9a1ad250b5b4 100644 --- a/planner/core/rule_eliminate_projection.go +++ b/planner/core/rule_eliminate_projection.go @@ -250,6 +250,9 @@ func (la *LogicalAggregation) replaceExprColumns(replace map[string]*expression. for _, aggExpr := range agg.Args { ResolveExprAndReplace(aggExpr, replace) } + for _, orderExpr := range agg.OrderByItems { + ResolveExprAndReplace(orderExpr.Expr, replace) + } } for _, gbyItem := range la.GroupByItems { ResolveExprAndReplace(gbyItem, replace)