Skip to content

Commit

Permalink
planner: fix group_concat function couldn't resolve the index of its …
Browse files Browse the repository at this point in the history
…order-by item (#46419) (#47119)

close #41986
  • Loading branch information
ti-chi-bot authored Oct 30, 2023
1 parent 78fc8b4 commit d4cafec
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 0 deletions.
12 changes: 12 additions & 0 deletions expression/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
}
35 changes: 35 additions & 0 deletions planner/core/rule_aggregation_push_down.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions planner/core/rule_eliminate_projection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit d4cafec

Please sign in to comment.