From d2236f4eff46c320780a8e57270f5414844d26ad Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Mon, 8 Jun 2020 18:48:08 +0800 Subject: [PATCH 1/7] expression: fix Const.DeferredExpr's wrong behavior --- expression/builtin_cast.go | 12 ++++++++++++ expression/explain.go | 3 +++ expression/expression.go | 7 ++++++- expression/integration_test.go | 22 ++++++++++++++++++++++ 4 files changed, 43 insertions(+), 1 deletion(-) diff --git a/expression/builtin_cast.go b/expression/builtin_cast.go index bf80d05178b7d..675c85ad7d82f 100644 --- a/expression/builtin_cast.go +++ b/expression/builtin_cast.go @@ -1798,6 +1798,18 @@ func BuildCastFunction(ctx sessionctx.Context, expr Expression, tp *types.FieldT RetType: tp, Function: f, } + + // We do not fold if the expression is Constant. And the Constant.DeferredExpr is not Constant. + constantExpr, isConst := expr.(*Constant) + if isConst { + deferredExpr := constantExpr.DeferredExpr + if deferredExpr != nil { + _, isConst := deferredExpr.(*Constant) + if !isConst { + return res + } + } + } // We do not fold CAST if the eval type of this scalar function is ETJson // since we may reset the flag of the field type of CastAsJson later which // would affect the evaluation of it. diff --git a/expression/explain.go b/expression/explain.go index b1994a426ba57..7cdf62a3afa10 100644 --- a/expression/explain.go +++ b/expression/explain.go @@ -62,6 +62,9 @@ func (col *Column) ExplainNormalizedInfo() string { // ExplainInfo implements the Expression interface. func (expr *Constant) ExplainInfo() string { + if expr.DeferredExpr != nil { + return expr.DeferredExpr.ExplainInfo() + } dt, err := expr.Eval(chunk.Row{}) if err != nil { return "not recognized const vanue" diff --git a/expression/expression.go b/expression/expression.go index 981eea995f1d8..e5a681ce78dc9 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -1062,7 +1062,12 @@ func canExprPushDown(expr Expression, pc PbConverter, storeType kv.StoreType) bo return false } switch x := expr.(type) { - case *Constant, *CorrelatedColumn: + case *CorrelatedColumn: + return pc.conOrCorColToPBExpr(expr) != nil + case *Constant: + if expr.(*Constant).DeferredExpr != nil { + return canExprPushDown(expr.(*Constant).DeferredExpr, pc, storeType) + } return pc.conOrCorColToPBExpr(expr) != nil case *Column: return pc.columnToPBExpr(x) != nil diff --git a/expression/integration_test.go b/expression/integration_test.go index a39f99b4644c9..d8ebfbe9fe2fb 100755 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -6597,3 +6597,25 @@ func (s *testIntegrationSuite) TestIssue17287(c *C) { tk.MustQuery("execute stmt7 using @val1;").Check(testkit.Rows("1589873945")) tk.MustQuery("execute stmt7 using @val2;").Check(testkit.Rows("1589873946")) } + +func (s *testIntegrationSuite) TestIssue17727(c *C) { + tk := testkit.NewTestKit(c, s.store) + orgEnable := plannercore.PreparedPlanCacheEnabled() + defer func() { + plannercore.SetPreparedPlanCache(orgEnable) + }() + plannercore.SetPreparedPlanCache(true) + var err error + tk.Se, err = session.CreateSession4TestWithOpt(s.store, &session.Opt{ + PreparedPlanCache: kvcache.NewSimpleLRUCache(100, 0.1, math.MaxUint64), + }) + c.Assert(err, IsNil) + + tk.MustExec("use test;") + tk.MustExec("DROP TABLE IF EXISTS t1;") + tk.MustExec("CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY auto_increment, a timestamp NOT NULL);") + tk.MustExec("INSERT INTO t1 VALUES (null, '2020-05-30 20:30:00');") + tk.MustExec("PREPARE mystmt FROM 'SELECT * FROM t1 WHERE UNIX_TIMESTAMP(a) >= ?';") + tk.MustExec("SET @a=1590868800;") + tk.MustQuery("EXECUTE mystmt USING @a;").Check(testkit.Rows()) +} From a2b60c20cf98b8b4a8e1ab31edb8a15ec28a893c Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Mon, 8 Jun 2020 19:05:04 +0800 Subject: [PATCH 2/7] clean the code --- expression/builtin_cast.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/expression/builtin_cast.go b/expression/builtin_cast.go index 675c85ad7d82f..ac00f4e4e4945 100644 --- a/expression/builtin_cast.go +++ b/expression/builtin_cast.go @@ -1799,15 +1799,12 @@ func BuildCastFunction(ctx sessionctx.Context, expr Expression, tp *types.FieldT Function: f, } - // We do not fold if the expression is Constant. And the Constant.DeferredExpr is not Constant. + // We do not fold if the expression is Constant and the Constant.DeferredExpr is not nil. constantExpr, isConst := expr.(*Constant) if isConst { deferredExpr := constantExpr.DeferredExpr if deferredExpr != nil { - _, isConst := deferredExpr.(*Constant) - if !isConst { - return res - } + return res } } // We do not fold CAST if the eval type of this scalar function is ETJson From 0f68d4681f384faf1aaaedd7fa12501a3be1cfc6 Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Tue, 9 Jun 2020 11:11:05 +0800 Subject: [PATCH 3/7] FIX UT --- executor/explainfor_test.go | 2 +- expression/expr_to_pb.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/executor/explainfor_test.go b/executor/explainfor_test.go index be99b72e4135f..6aeb5bb54a589 100644 --- a/executor/explainfor_test.go +++ b/executor/explainfor_test.go @@ -181,7 +181,7 @@ func (s *testPrepareSerialSuite) TestExplainForConnPlanCache(c *C) { tk.Se.SetSessionManager(&mockSessionManager1{PS: ps}) tk.MustQuery(fmt.Sprintf("explain for connection %s", connID)).Check(testkit.Rows( "TableReader_7 8000.00 root data:Selection_6", - "└─Selection_6 8000.00 cop[tikv] eq(cast(test.t.a), 1)", + "└─Selection_6 8000.00 cop[tikv] eq(cast(test.t.a), cast(\"1\"))", " └─TableFullScan_5 10000.00 cop[tikv] table:t keep order:false, stats:pseudo", )) } diff --git a/expression/expr_to_pb.go b/expression/expr_to_pb.go index a68673f1b4a19..a0b85fd44a0df 100644 --- a/expression/expr_to_pb.go +++ b/expression/expr_to_pb.go @@ -59,6 +59,9 @@ func NewPBConverter(client kv.Client, sc *stmtctx.StatementContext) PbConverter func (pc PbConverter) ExprToPB(expr Expression) *tipb.Expr { switch x := expr.(type) { case *Constant: + if expr.(*Constant).DeferredExpr != nil { + return pc.ExprToPB(expr.(*Constant).DeferredExpr) + } pbExpr := pc.conOrCorColToPBExpr(expr) if pbExpr == nil { return nil From 17b9ead801426fb6f61d21999a4b611b252074df Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Tue, 9 Jun 2020 14:30:47 +0800 Subject: [PATCH 4/7] revert the change and add check for `funcCallToExpression` --- executor/explainfor_test.go | 2 +- expression/builtin_cast.go | 9 --------- expression/explain.go | 3 --- expression/expr_to_pb.go | 3 --- expression/expression.go | 7 +------ planner/core/expression_rewriter.go | 13 ++++++++++--- 6 files changed, 12 insertions(+), 25 deletions(-) diff --git a/executor/explainfor_test.go b/executor/explainfor_test.go index 6aeb5bb54a589..be99b72e4135f 100644 --- a/executor/explainfor_test.go +++ b/executor/explainfor_test.go @@ -181,7 +181,7 @@ func (s *testPrepareSerialSuite) TestExplainForConnPlanCache(c *C) { tk.Se.SetSessionManager(&mockSessionManager1{PS: ps}) tk.MustQuery(fmt.Sprintf("explain for connection %s", connID)).Check(testkit.Rows( "TableReader_7 8000.00 root data:Selection_6", - "└─Selection_6 8000.00 cop[tikv] eq(cast(test.t.a), cast(\"1\"))", + "└─Selection_6 8000.00 cop[tikv] eq(cast(test.t.a), 1)", " └─TableFullScan_5 10000.00 cop[tikv] table:t keep order:false, stats:pseudo", )) } diff --git a/expression/builtin_cast.go b/expression/builtin_cast.go index ac00f4e4e4945..bf80d05178b7d 100644 --- a/expression/builtin_cast.go +++ b/expression/builtin_cast.go @@ -1798,15 +1798,6 @@ func BuildCastFunction(ctx sessionctx.Context, expr Expression, tp *types.FieldT RetType: tp, Function: f, } - - // We do not fold if the expression is Constant and the Constant.DeferredExpr is not nil. - constantExpr, isConst := expr.(*Constant) - if isConst { - deferredExpr := constantExpr.DeferredExpr - if deferredExpr != nil { - return res - } - } // We do not fold CAST if the eval type of this scalar function is ETJson // since we may reset the flag of the field type of CastAsJson later which // would affect the evaluation of it. diff --git a/expression/explain.go b/expression/explain.go index 7cdf62a3afa10..b1994a426ba57 100644 --- a/expression/explain.go +++ b/expression/explain.go @@ -62,9 +62,6 @@ func (col *Column) ExplainNormalizedInfo() string { // ExplainInfo implements the Expression interface. func (expr *Constant) ExplainInfo() string { - if expr.DeferredExpr != nil { - return expr.DeferredExpr.ExplainInfo() - } dt, err := expr.Eval(chunk.Row{}) if err != nil { return "not recognized const vanue" diff --git a/expression/expr_to_pb.go b/expression/expr_to_pb.go index a0b85fd44a0df..a68673f1b4a19 100644 --- a/expression/expr_to_pb.go +++ b/expression/expr_to_pb.go @@ -59,9 +59,6 @@ func NewPBConverter(client kv.Client, sc *stmtctx.StatementContext) PbConverter func (pc PbConverter) ExprToPB(expr Expression) *tipb.Expr { switch x := expr.(type) { case *Constant: - if expr.(*Constant).DeferredExpr != nil { - return pc.ExprToPB(expr.(*Constant).DeferredExpr) - } pbExpr := pc.conOrCorColToPBExpr(expr) if pbExpr == nil { return nil diff --git a/expression/expression.go b/expression/expression.go index e5a681ce78dc9..981eea995f1d8 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -1062,12 +1062,7 @@ func canExprPushDown(expr Expression, pc PbConverter, storeType kv.StoreType) bo return false } switch x := expr.(type) { - case *CorrelatedColumn: - return pc.conOrCorColToPBExpr(expr) != nil - case *Constant: - if expr.(*Constant).DeferredExpr != nil { - return canExprPushDown(expr.(*Constant).DeferredExpr, pc, storeType) - } + case *Constant, *CorrelatedColumn: return pc.conOrCorColToPBExpr(expr) != nil case *Column: return pc.columnToPBExpr(x) != nil diff --git a/planner/core/expression_rewriter.go b/planner/core/expression_rewriter.go index 74b7117c89b14..9808179cd2d2b 100644 --- a/planner/core/expression_rewriter.go +++ b/planner/core/expression_rewriter.go @@ -1525,9 +1525,16 @@ func (er *expressionRewriter) funcCallToExpression(v *ast.FuncCallExpr) { var function expression.Expression er.ctxStackPop(len(v.Args)) if _, ok := expression.DeferredFunctions[v.FnName.L]; er.useCache() && ok { - function, er.err = expression.NewFunctionBase(er.sctx, v.FnName.L, &v.Type, args...) - c := &expression.Constant{Value: types.NewDatum(nil), RetType: function.GetType().Clone(), DeferredExpr: function} - er.ctxStackAppend(c, types.EmptyName) + // When the expression is unix_timestamp and the length of argument is not zero, + // we deal with it as normal expression. + if v.FnName.L == ast.UnixTimestamp && len(v.Args) != 0 { + function, er.err = er.newFunction(v.FnName.L, &v.Type, args...) + er.ctxStackAppend(function, types.EmptyName) + } else { + function, er.err = expression.NewFunctionBase(er.sctx, v.FnName.L, &v.Type, args...) + c := &expression.Constant{Value: types.NewDatum(nil), RetType: function.GetType().Clone(), DeferredExpr: function} + er.ctxStackAppend(c, types.EmptyName) + } } else { function, er.err = er.newFunction(v.FnName.L, &v.Type, args...) er.ctxStackAppend(function, types.EmptyName) From 76c9869524ebfee2b8a93443d23dbf011c4245d6 Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Fri, 12 Jun 2020 13:33:51 +0800 Subject: [PATCH 5/7] solve conflicts --- expression/integration_test.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/expression/integration_test.go b/expression/integration_test.go index 97835c17a9356..eaf3dfe8cf00d 100755 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -6620,6 +6620,28 @@ func (s *testIntegrationSuite) TestIssue17287(c *C) { tk.MustQuery("execute stmt7 using @val2;").Check(testkit.Rows("1589873946")) } +func (s *testIntegrationSuite) TestIssue17727(c *C) { + tk := testkit.NewTestKit(c, s.store) + orgEnable := plannercore.PreparedPlanCacheEnabled() + defer func() { + plannercore.SetPreparedPlanCache(orgEnable) + }() + plannercore.SetPreparedPlanCache(true) + var err error + tk.Se, err = session.CreateSession4TestWithOpt(s.store, &session.Opt{ + PreparedPlanCache: kvcache.NewSimpleLRUCache(100, 0.1, math.MaxUint64), + }) + c.Assert(err, IsNil) + + tk.MustExec("use test;") + tk.MustExec("DROP TABLE IF EXISTS t1;") + tk.MustExec("CREATE TABLE t1 (id INT NOT NULL PRIMARY KEY auto_increment, a timestamp NOT NULL);") + tk.MustExec("INSERT INTO t1 VALUES (null, '2020-05-30 20:30:00');") + tk.MustExec("PREPARE mystmt FROM 'SELECT * FROM t1 WHERE UNIX_TIMESTAMP(a) >= ?';") + tk.MustExec("SET @a=1590868800;") + tk.MustQuery("EXECUTE mystmt USING @a;").Check(testkit.Rows()) +} + func (s *testIntegrationSerialSuite) TestIssue17891(c *C) { collate.SetNewCollationEnabledForTest(true) defer collate.SetNewCollationEnabledForTest(false) From 7abecb224540fd83aadc9a871176f663b1ac59cf Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Fri, 12 Jun 2020 13:35:34 +0800 Subject: [PATCH 6/7] fix typo --- planner/core/expression_rewriter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/planner/core/expression_rewriter.go b/planner/core/expression_rewriter.go index 946d9e3456b4f..780605001ac12 100644 --- a/planner/core/expression_rewriter.go +++ b/planner/core/expression_rewriter.go @@ -1526,7 +1526,7 @@ func (er *expressionRewriter) funcCallToExpression(v *ast.FuncCallExpr) { var function expression.Expression er.ctxStackPop(len(v.Args)) if _, ok := expression.DeferredFunctions[v.FnName.L]; er.useCache() && ok { - // When the expression is unix_timestamp and the length of argument is not zero, + // When the expression is unix_timestamp and the number of argument is not zero, // we deal with it as normal expression. if v.FnName.L == ast.UnixTimestamp && len(v.Args) != 0 { function, er.err = er.newFunction(v.FnName.L, &v.Type, args...) From 39db566bc15e5b168cf94b52061d9792a28b757b Mon Sep 17 00:00:00 2001 From: Reminiscent Date: Mon, 15 Jun 2020 10:27:16 +0800 Subject: [PATCH 7/7] address comments --- expression/integration_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/expression/integration_test.go b/expression/integration_test.go index eaf3dfe8cf00d..ac699115c03f8 100755 --- a/expression/integration_test.go +++ b/expression/integration_test.go @@ -6640,6 +6640,20 @@ func (s *testIntegrationSuite) TestIssue17727(c *C) { tk.MustExec("PREPARE mystmt FROM 'SELECT * FROM t1 WHERE UNIX_TIMESTAMP(a) >= ?';") tk.MustExec("SET @a=1590868800;") tk.MustQuery("EXECUTE mystmt USING @a;").Check(testkit.Rows()) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + + tk.MustExec("SET @a=1590868801;") + tk.MustQuery("EXECUTE mystmt USING @a;").Check(testkit.Rows()) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) + + tk.MustExec("prepare stmt from 'select unix_timestamp(?)';") + tk.MustExec("set @a = '2020-05-30 20:30:00';") + tk.MustQuery("execute stmt using @a;").Check(testkit.Rows("1590841800")) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("0")) + + tk.MustExec("set @a = '2020-06-12 13:47:58';") + tk.MustQuery("execute stmt using @a;").Check(testkit.Rows("1591940878")) + tk.MustQuery("select @@last_plan_from_cache;").Check(testkit.Rows("1")) } func (s *testIntegrationSerialSuite) TestIssue17891(c *C) {