Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner: projection should not push the expr that is not fully substituted #38802

Merged
merged 8 commits into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions expression/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7768,6 +7768,23 @@ func TestJSONStorageFree(t *testing.T) {
require.Error(t, err, "[json:3140]Invalid JSON text: The document root must not be followed by other values.")
}

func TestIssue38736(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
tk.MustExec("use test")
tk.MustExec("CREATE TABLE t0(c0 BOOL, c1 INT);")
tk.MustExec("CREATE TABLE t1 LIKE t0;")
tk.MustExec("CREATE definer='root'@'localhost' VIEW v0(c0) AS SELECT IS_IPV4(t0.c1) FROM t0, t1;")
tk.MustExec("INSERT INTO t0(c0, c1) VALUES (true, 0);")
tk.MustExec("INSERT INTO t1(c0, c1) VALUES (true, 2);")

// The filter is evaled as false.
tk.MustQuery("SELECT v0.c0 FROM v0 WHERE (v0.c0)NOT LIKE(BINARY v0.c0);").Check(testkit.Rows())
Reminiscent marked this conversation as resolved.
Show resolved Hide resolved

// Also the filter is evaled as false.
tk.MustQuery("SELECT v0.c0 FROM v0 WHERE (v0.c0)NOT LIKE(BINARY v0.c0) or v0.c0 > 0").Check(testkit.Rows())
}

func TestJSONExtractFromLast(t *testing.T) {
store := testkit.CreateMockStore(t)
tk := testkit.NewTestKit(t, store)
Expand Down
53 changes: 22 additions & 31 deletions expression/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (c *cowExprRef) Set(i int, changed bool, val Expression) {
return
}
c.new = make([]Expression, len(c.ref))
copy(c.new, c.ref[:i])
copy(c.new, c.ref)
c.new[i] = val
}

Expand Down Expand Up @@ -382,17 +382,6 @@ func SetExprColumnInOperand(expr Expression) Expression {
return expr
}

// ColumnSubstitute4PPD substitutes the columns in filter to expressions in select fields.
// Only used for predicate push down to projection. Some columns can not be substituted for some reasons.
// So we should return the bool value to indicate some expressions can not be pushed down.
// e.g. CREATE TABLE t3(c0 INT, primary key(c0));
// SELECT v2.c0 FROM (select 1 as c0 from t3) v2 WHERE (v2.c0)like(True);
// The cond `(v2.c0)like(True)` can not be substituted when the new collation enable. So we shouldn't push the cond down to the projection.
func ColumnSubstitute4PPD(expr Expression, schema *Schema, newExprs []Expression) (bool, Expression) {
substituted, _, resExpr := ColumnSubstituteImpl(expr, schema, newExprs, false)
return substituted, resExpr
}

// 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.
func ColumnSubstitute(expr Expression, schema *Schema, newExprs []Expression) Expression {
Expand Down Expand Up @@ -432,41 +421,43 @@ func ColumnSubstituteImpl(expr Expression, schema *Schema, newExprs []Expression
substituted := false
hasFail := false
if v.FuncName.L == ast.Cast {
newFunc := v.Clone().(*ScalarFunction)
substituted, hasFail, newFunc.GetArgs()[0] = ColumnSubstituteImpl(newFunc.GetArgs()[0], schema, newExprs, fail1Return)
var newArg Expression
substituted, hasFail, newArg = ColumnSubstituteImpl(v.GetArgs()[0], schema, newExprs, fail1Return)
if fail1Return && hasFail {
return substituted, hasFail, newFunc
return substituted, hasFail, v
}
if substituted {
// Workaround for issue https://github.com/pingcap/tidb/issues/28804
e := NewFunctionInternal(v.GetCtx(), v.FuncName.L, v.RetType, newFunc.GetArgs()...)
e := BuildCastFunction(v.GetCtx(), newArg, v.RetType)
e.SetCoercibility(v.Coercibility())
return true, false, e
}
return false, false, newFunc
return false, false, v
}
// cowExprRef is a copy-on-write util, args array allocation happens only
// when expr in args is changed
refExprArr := cowExprRef{v.GetArgs(), nil}
_, coll := DeriveCollationFromExprs(v.GetCtx(), v.GetArgs()...)
var tmpArgForCollCheck []Expression
if collate.NewCollationEnabled() {
tmpArgForCollCheck = make([]Expression, len(v.GetArgs()))
}
for idx, arg := range v.GetArgs() {
changed, hasFail, newFuncExpr := ColumnSubstituteImpl(arg, schema, newExprs, fail1Return)
if fail1Return && hasFail {
return changed, hasFail, v
changed, failed, newFuncExpr := ColumnSubstituteImpl(arg, schema, newExprs, fail1Return)
if fail1Return && failed {
return changed, failed, v
}
oldChanged := changed
if collate.NewCollationEnabled() {
if collate.NewCollationEnabled() && changed {
// Make sure the collation used by the ScalarFunction isn't changed and its result collation is not weaker than the collation used by the ScalarFunction.
if changed {
changed = false
tmpArgs := make([]Expression, 0, len(v.GetArgs()))
_ = append(append(append(tmpArgs, refExprArr.Result()[0:idx]...), refExprArr.Result()[idx+1:]...), newFuncExpr)
_, newColl := DeriveCollationFromExprs(v.GetCtx(), append(v.GetArgs(), newFuncExpr)...)
if coll == newColl {
changed = checkCollationStrictness(coll, newFuncExpr.GetType().GetCollate())
}
changed = false
copy(tmpArgForCollCheck, refExprArr.Result())
tmpArgForCollCheck[idx] = newFuncExpr
_, newColl := DeriveCollationFromExprs(v.GetCtx(), tmpArgForCollCheck...)
if coll == newColl {
changed = checkCollationStrictness(coll, newFuncExpr.GetType().GetCollate())
}
}
hasFail = hasFail || failed || oldChanged != changed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since fail1Return is true
once failed is true, it should be returned from L447 already.
once oldChanged != changed is true, it should be returned from L467 soon.

hasFail seems always false here below

if fail1Return && oldChanged != changed {
// Only when the oldChanged is true and changed is false, we will get here.
// And this means there some dependency in this arg can be substituted with
Expand All @@ -481,7 +472,7 @@ func ColumnSubstituteImpl(expr Expression, schema *Schema, newExprs []Expression
}
}
if substituted {
return true, false, NewFunctionInternal(v.GetCtx(), v.FuncName.L, v.RetType, refExprArr.Result()...)
return true, hasFail, NewFunctionInternal(v.GetCtx(), v.FuncName.L, v.RetType, refExprArr.Result()...)
}
}
return false, false, expr
Expand Down
4 changes: 2 additions & 2 deletions planner/cascades/transformation_rules.go
Original file line number Diff line number Diff line change
Expand Up @@ -550,8 +550,8 @@ func (*PushSelDownProjection) OnTransform(old *memo.ExprIter) (newExprs []*memo.
canBePushed := make([]expression.Expression, 0, len(sel.Conditions))
canNotBePushed := make([]expression.Expression, 0, len(sel.Conditions))
for _, cond := range sel.Conditions {
substituted, newFilter := expression.ColumnSubstitute4PPD(cond, projSchema, proj.Exprs)
if substituted && !expression.HasGetSetVarFunc(newFilter) {
substituted, hasFailed, newFilter := expression.ColumnSubstituteImpl(cond, projSchema, proj.Exprs, true)
if substituted && !hasFailed && !expression.HasGetSetVarFunc(newFilter) {
canBePushed = append(canBePushed, newFilter)
} else {
canNotBePushed = append(canNotBePushed, cond)
Expand Down
4 changes: 2 additions & 2 deletions planner/core/rule_predicate_push_down.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,8 +474,8 @@ func (p *LogicalProjection) PredicatePushDown(predicates []expression.Expression
}
}
for _, cond := range predicates {
substituted, newFilter := expression.ColumnSubstitute4PPD(cond, p.Schema(), p.Exprs)
if substituted && !expression.HasGetSetVarFunc(newFilter) {
substituted, hasFailed, newFilter := expression.ColumnSubstituteImpl(cond, p.Schema(), p.Exprs, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we can remove the function ColumnSubstitute4PPD.

if substituted && !hasFailed && !expression.HasGetSetVarFunc(newFilter) {
canBePushed = append(canBePushed, newFilter)
} else {
canNotBePushed = append(canNotBePushed, cond)
Expand Down