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 (pingcap#46419) (pingcap#47120)

close pingcap#41986
  • Loading branch information
ti-chi-bot authored Oct 30, 2023
1 parent 02d7703 commit 62766de
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 0 deletions.
11 changes: 11 additions & 0 deletions expression/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
}
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 @@ -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)
Expand All @@ -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 {
Expand All @@ -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
}
Expand All @@ -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)
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 @@ -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)
Expand Down

0 comments on commit 62766de

Please sign in to comment.