From d4cafec23f4afd9ba270acf3f7fe16c6a5cc0020 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Mon, 30 Oct 2023 16:05:07 +0800 Subject: [PATCH] planner: fix group_concat function couldn't resolve the index of its order-by item (#46419) (#47119) close pingcap/tidb#41986 --- expression/integration_test.go | 12 ++++++++ planner/core/rule_aggregation_push_down.go | 35 ++++++++++++++++++++++ planner/core/rule_eliminate_projection.go | 3 ++ 3 files changed, 50 insertions(+) diff --git a/expression/integration_test.go b/expression/integration_test.go index fc791da3dcce8..4807cb22aa5d1 100644 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -7484,3 +7484,15 @@ 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, clean := testkit.CreateMockStore(t) + defer clean() + 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 b6c0029f7a37b..8038a45f62132 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" ) @@ -519,6 +520,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) @@ -531,6 +534,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 { @@ -540,6 +564,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 } @@ -548,6 +582,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 68a20d10f59b9..87613baeec521 100644 --- a/planner/core/rule_eliminate_projection.go +++ b/planner/core/rule_eliminate_projection.go @@ -259,6 +259,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)