From 9eefe931de1bc5956a09c45ce84881dcde8c92d9 Mon Sep 17 00:00:00 2001 From: AilinKid <314806019@qq.com> Date: Fri, 25 Aug 2023 14:28:25 +0800 Subject: [PATCH 1/5] fix group_concat function couldn't resolve the index of its order-by item Signed-off-by: AilinKid <314806019@qq.com> --- .../integration_test/integration_test.go | 11 +++++++++ planner/core/rule_aggregation_push_down.go | 24 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/expression/integration_test/integration_test.go b/expression/integration_test/integration_test.go index 0222b3aaa1782..5901c4c56b15f 100644 --- a/expression/integration_test/integration_test.go +++ b/expression/integration_test/integration_test.go @@ -2048,3 +2048,14 @@ func TestCompareBuiltin(t *testing.T) { result = tk.MustQuery(`select row(1+3,2,3)<>row(1+3,2,3)`) result.Check(testkit.Rows("0")) } + +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 d262787c51235..5c7664edf0808 100644 --- a/planner/core/rule_aggregation_push_down.go +++ b/planner/core/rule_aggregation_push_down.go @@ -18,6 +18,7 @@ import ( "bytes" "context" "fmt" + "github.com/pingcap/tidb/planner/util" "github.com/pingcap/tidb/expression" "github.com/pingcap/tidb/expression/aggregation" @@ -559,6 +560,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) @@ -566,11 +569,25 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim for _, arg := range aggFunc.Args { newArgs = append(newArgs, expression.ColumnSubstitute(arg, proj.schema, proj.Exprs)) } + 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(newArgs) { noSideEffects = false break } + if ExprsHasSideEffects(newOrderByItems) { + noSideEffects = false + break + } newAggFuncsArgs = append(newAggFuncsArgs, newArgs) + 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) } } for i, funcsArgs := range oldAggFuncsArgs { @@ -580,6 +597,12 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim break } } + for j := range newAggOrderItems { + if oldAggOrderItems[i][j].Expr.GetType().EvalType() != newAggOrderItems[i][j].Expr.GetType().EvalType() { + noSideEffects = false + break + } + } if !noSideEffects { break } @@ -588,6 +611,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) From c271d0918ff6bb18713c46c9aec8f2595b17e00d Mon Sep 17 00:00:00 2001 From: AilinKid <314806019@qq.com> Date: Fri, 25 Aug 2023 14:30:54 +0800 Subject: [PATCH 2/5] make fmt Signed-off-by: AilinKid <314806019@qq.com> --- planner/core/rule_aggregation_push_down.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/rule_aggregation_push_down.go b/planner/core/rule_aggregation_push_down.go index 5c7664edf0808..dc346602371d0 100644 --- a/planner/core/rule_aggregation_push_down.go +++ b/planner/core/rule_aggregation_push_down.go @@ -18,12 +18,12 @@ import ( "bytes" "context" "fmt" - "github.com/pingcap/tidb/planner/util" "github.com/pingcap/tidb/expression" "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" ) From c355d8399c69c4273ee2ddd663d3dbec4d6f7bf2 Mon Sep 17 00:00:00 2001 From: AilinKid <314806019@qq.com> Date: Wed, 6 Sep 2023 18:52:46 +0800 Subject: [PATCH 3/5] refine Signed-off-by: AilinKid <314806019@qq.com> --- planner/core/rule_aggregation_push_down.go | 38 ++++++++++++++-------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/planner/core/rule_aggregation_push_down.go b/planner/core/rule_aggregation_push_down.go index dc346602371d0..11282fb50aae0 100644 --- a/planner/core/rule_aggregation_push_down.go +++ b/planner/core/rule_aggregation_push_down.go @@ -569,25 +569,31 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim for _, arg := range aggFunc.Args { newArgs = append(newArgs, expression.ColumnSubstitute(arg, proj.schema, proj.Exprs)) } - 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(newArgs) { noSideEffects = false break } - if ExprsHasSideEffects(newOrderByItems) { - noSideEffects = false - break - } newAggFuncsArgs = append(newAggFuncsArgs, newArgs) - 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}) + 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) } - newAggOrderItems = append(newAggOrderItems, oneAggOrderByItems) } } for i, funcsArgs := range oldAggFuncsArgs { @@ -597,7 +603,11 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim break } } - for j := range newAggOrderItems { + 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 From f9456a211ee09ea5343e4da0c2dc98448dcc1039 Mon Sep 17 00:00:00 2001 From: AilinKid <314806019@qq.com> Date: Tue, 19 Sep 2023 18:56:59 +0800 Subject: [PATCH 4/5] address comment Signed-off-by: AilinKid <314806019@qq.com> --- planner/core/rule_aggregation_push_down.go | 1 + 1 file changed, 1 insertion(+) diff --git a/planner/core/rule_aggregation_push_down.go b/planner/core/rule_aggregation_push_down.go index 11282fb50aae0..714e7dae2f126 100644 --- a/planner/core/rule_aggregation_push_down.go +++ b/planner/core/rule_aggregation_push_down.go @@ -574,6 +574,7 @@ 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)) From b0bcb3ea245f350bd9b28b01ea3ab6f90e5ce816 Mon Sep 17 00:00:00 2001 From: AilinKid <314806019@qq.com> Date: Tue, 19 Sep 2023 19:29:52 +0800 Subject: [PATCH 5/5] make bazel Signed-off-by: AilinKid <314806019@qq.com> --- expression/integration_test/BUILD.bazel | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/expression/integration_test/BUILD.bazel b/expression/integration_test/BUILD.bazel index 3db2f1147ce2d..7305f7f9d7a86 100644 --- a/expression/integration_test/BUILD.bazel +++ b/expression/integration_test/BUILD.bazel @@ -8,7 +8,7 @@ go_test( "main_test.go", ], flaky = True, - shard_count = 26, + shard_count = 27, deps = [ "//config", "//domain",