Skip to content

Commit

Permalink
planner: stop pushing Agg down through Projection if substitution fail (
Browse files Browse the repository at this point in the history
  • Loading branch information
ti-chi-bot authored Mar 28, 2024
1 parent 0cc48c8 commit a824267
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 6 deletions.
1 change: 1 addition & 0 deletions expression/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,7 @@ func SetExprColumnInOperand(expr Expression) Expression {

// ColumnSubstitute substitutes the columns in filter to expressions in select fields.
// e.g. select * from (select b as a from t) k where a < 10 => select * from (select b as a from t where b < 10) k.
// TODO: remove this function and only use ColumnSubstituteImpl since this function swallows the error, which seems unsafe.
func ColumnSubstitute(expr Expression, schema *Schema, newExprs []Expression) Expression {
_, _, resExpr := ColumnSubstituteImpl(expr, schema, newExprs, false)
return resExpr
Expand Down
9 changes: 9 additions & 0 deletions planner/core/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6946,6 +6946,15 @@ func TestIssue32632(t *testing.T) {
tk.MustExec("drop table if exists supplier")
}

func TestIssue50926(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("use test")
tk.MustExec("create table t (a int)")
tk.MustExec("create or replace definer='root'@'localhost' view v (a,b) AS select 1 as a, json_object('k', '0') as b from t")
tk.MustQuery("select sum(json_extract(b, '$.path')) from v group by a").Check(testkit.Rows()) // no error
}

func TestTiFlashPartitionTableScan(t *testing.T) {
failpoint.Enable("github.com/pingcap/tidb/planner/core/forceDynamicPrune", `return(true)`)
defer failpoint.Disable("github.com/pingcap/tidb/planner/core/forceDynamicPrune")
Expand Down
27 changes: 21 additions & 6 deletions planner/core/rule_aggregation_push_down.go
Original file line number Diff line number Diff line change
Expand Up @@ -546,7 +546,12 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim
noSideEffects := true
newGbyItems := make([]expression.Expression, 0, len(agg.GroupByItems))
for _, gbyItem := range agg.GroupByItems {
newGbyItems = append(newGbyItems, expression.ColumnSubstitute(gbyItem, proj.schema, proj.Exprs))
_, failed, groupBy := expression.ColumnSubstituteImpl(gbyItem, proj.schema, proj.Exprs, true)
if failed {
noSideEffects = false
break
}
newGbyItems = append(newGbyItems, groupBy)
if ExprsHasSideEffects(newGbyItems) {
noSideEffects = false
break
Expand All @@ -561,7 +566,12 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim
oldAggFuncsArgs = append(oldAggFuncsArgs, aggFunc.Args)
newArgs := make([]expression.Expression, 0, len(aggFunc.Args))
for _, arg := range aggFunc.Args {
newArgs = append(newArgs, expression.ColumnSubstitute(arg, proj.schema, proj.Exprs))
_, failed, newArg := expression.ColumnSubstituteImpl(arg, proj.schema, proj.Exprs, true)
if failed {
noSideEffects = false
break
}
newArgs = append(newArgs, newArg)
}
if ExprsHasSideEffects(newArgs) {
noSideEffects = false
Expand All @@ -573,7 +583,12 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim
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))
_, failed, byItem := expression.ColumnSubstituteImpl(oby.Expr, proj.schema, proj.Exprs, true)
if failed {
noSideEffects = false
break
}
newOrderByItems = append(newOrderByItems, byItem)
}
if ExprsHasSideEffects(newOrderByItems) {
noSideEffects = false
Expand All @@ -592,6 +607,9 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim
}
}
for i, funcsArgs := range oldAggFuncsArgs {
if !noSideEffects {
break
}
for j := range funcsArgs {
if oldAggFuncsArgs[i][j].GetType().EvalType() != newAggFuncsArgs[i][j].GetType().EvalType() {
noSideEffects = false
Expand All @@ -608,9 +626,6 @@ func (a *aggregationPushDownSolver) aggPushDown(p LogicalPlan, opt *logicalOptim
break
}
}
if !noSideEffects {
break
}
}
if noSideEffects {
agg.GroupByItems = newGbyItems
Expand Down

0 comments on commit a824267

Please sign in to comment.