expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache #8105

dbjoa · 2018-10-30T06:04:10Z

What problem does this PR solve?

Support non-deterministic functions (e.g., now) in the prepared plan cache.

What is changed and how it works?

Make a non-deterministic function expression to be a constant expression with the deferred expression (i.e., the non-deterministic function expression).

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has exported variable/fields change
Has interface methods change

Side effects

No

Related changes

Need to be included in the release note

…n the plan cache

sre-bot · 2018-10-30T06:04:14Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

dbjoa · 2018-10-30T06:04:35Z

/run-all-tests

shenli · 2018-10-30T06:11:39Z

@dbjoa Thanks!

eurekaka

Nice work, LGTM.

eurekaka · 2018-10-31T01:58:02Z

expression/scalar_function.go

@@ -70,8 +70,8 @@ func (sf *ScalarFunction) MarshalJSON() ([]byte, error) {
 	return []byte(fmt.Sprintf("\"%s\"", sf)), nil
 }

-// NewFunction creates a new scalar function or constant.
-func NewFunction(ctx sessionctx.Context, funcName string, retType *types.FieldType, args ...Expression) (Expression, error) {
+// newFunction creates a new scalar function or constant.


s/newFunction/newFunctionImpl

Thank you for the comment. I've updated the PR.

XuHuaiyu · 2018-10-31T07:35:45Z

planner/core/prepare_test.go

 	"github.com/pingcap/tidb/planner/core"
 	"github.com/pingcap/tidb/util/testkit"
 	"github.com/pingcap/tidb/util/testleak"
+	dto "github.com/prometheus/client_model/go"
+	"time"


put this line to line 17 and leave a blank line behind it.

Thank you for the detailed comment. I've updated the PR.

XuHuaiyu · 2018-10-31T07:37:26Z

expression/scalar_function.go

-// NewFunction creates a new scalar function or constant.
-func NewFunction(ctx sessionctx.Context, funcName string, retType *types.FieldType, args ...Expression) (Expression, error) {
+// newFunctionImpl creates a new scalar function or constant.
+func newFunctionImpl(fold bool, ctx sessionctx.Context, funcName string, retType *types.FieldType, args ...Expression) (Expression, error) {


ctx is always the first argument of a function,
we'd better keep this style.

Thank you for the detailed comment. I've updated the PR.

XuHuaiyu · 2018-10-31T08:19:25Z

MySQL:

mysql> prepare stmt from "select now(), sleep(1), now(), sleep(1), now()";
Query OK, 0 rows affected (0.00 sec)
Statement prepared

mysql> execute stmt;
+---------------------+----------+---------------------+----------+----------------$----+
| now()               | sleep(1) | now()               | sleep(1) | now()
    |
+---------------------+----------+---------------------+----------+----------------$----+
| 2018-10-31 16:00:14 |        0 | 2018-10-31 16:00:14 |        0 | 2018-10-31 16:0$:14 |
+---------------------+----------+---------------------+----------+----------------$----+
1 row in set (2.01 sec)

mysql> execute stmt;
+---------------------+----------+---------------------+----------+----------------$----+
| now()               | sleep(1) | now()               | sleep(1) | now()
    |
+---------------------+----------+---------------------+----------+----------------$----+
| 2018-10-31 16:00:19 |        0 | 2018-10-31 16:00:19 |        0 | 2018-10-31 16:0$:19 |
+---------------------+----------+---------------------+----------+----------------$----+
1 row in set (2.01 sec)

In MySQL, now is a foldable function,
but It seems we will get different results for different now() with this commit?

dbjoa · 2018-10-31T12:02:15Z

@XuHuaiyu, Thank you for the comment. The updated PR should address the issue.

dbjoa · 2018-10-31T12:02:25Z

/run-all-tests

dbjoa · 2018-10-31T13:34:04Z

/run-all-tests

dbjoa · 2018-10-31T15:19:22Z

I should also handle the synonyms of NOW() (i.e., CURDATE(), CURTIME(), UTC_DATE(), UTC_TIME(), UTC_TIMESTAMP(), CURRENT_TIMESTAMP(), UNIX_TIMESTAMP()).

CLAassistant · 2018-10-31T15:52:34Z

All committers have signed the CLA.

dbjoa · 2018-10-31T15:53:48Z

/run-all-tests

dbjoa · 2018-10-31T16:08:46Z

The updated PR should handle the synonyms of NOW().

dbjoa · 2018-10-31T21:24:53Z

/run-all-tests

zz-jason · 2018-11-01T10:38:26Z

planner/core/prepare_test.go

@@ -14,7 +14,13 @@
 package core_test

 import (
+	dto "github.com/prometheus/client_model/go"


please move this package to the lower block. separate it from the golang standard packages.

Thank you for the comment. I've updated the PR.

zz-jason · 2018-11-01T10:40:20Z

expression/function_traits.go

@@ -52,6 +41,23 @@ var unFoldableFunctions = map[string]struct{}{
 	ast.GetParam:  {},
 }

+// DeferredFunctions stores non-deterministic functions which can be deferred.


How adding another comment to specify that the deferred functions take effects only when plan cache is enabled?

Thank you for the comment. I've added the comment to the updated the PR.

zz-jason

LGTM

zz-jason · 2018-11-01T15:25:13Z

/run-all-tests

expression,planner: support non-deterministic functions (e.g., now) i…

1db4a0e

…n the plan cache

shenli added the contribution This PR is from a community contributor. label Oct 30, 2018

eurekaka added type/enhancement The issue or PR belongs to an enhancement. sig/planner SIG: Planner status/all tests passed labels Oct 30, 2018

eurekaka reviewed Oct 31, 2018

View reviewed changes

eurekaka added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 31, 2018

XuHuaiyu reviewed Oct 31, 2018

View reviewed changes

dbjoa changed the title ~~expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache~~ [WIP] expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache Oct 31, 2018

dbjoa changed the title ~~[WIP] expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache~~ expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache Oct 31, 2018

zz-jason reviewed Nov 1, 2018

View reviewed changes

zz-jason approved these changes Nov 1, 2018

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 1, 2018

ngaut merged commit 5374ff8 into pingcap:master Nov 1, 2018

dbjoa mentioned this pull request Dec 10, 2018

expression,planner: fix to preserve the precision information of a timestamp-typed column in the plan cache #8619

Merged

eurekaka mentioned this pull request Apr 9, 2020

expression: avoid Order By constant expression error when plan cache is enabled #16261

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache #8105

expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache #8105

dbjoa commented Oct 30, 2018

sre-bot commented Oct 30, 2018

dbjoa commented Oct 30, 2018

shenli commented Oct 30, 2018

eurekaka left a comment

eurekaka Oct 31, 2018

dbjoa Oct 31, 2018

XuHuaiyu Oct 31, 2018

dbjoa Oct 31, 2018

XuHuaiyu Oct 31, 2018

dbjoa Oct 31, 2018

XuHuaiyu commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018 •

edited

Loading

CLAassistant commented Oct 31, 2018 •

edited

Loading

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

zz-jason Nov 1, 2018

dbjoa Nov 1, 2018

zz-jason Nov 1, 2018

dbjoa Nov 1, 2018

zz-jason left a comment

zz-jason commented Nov 1, 2018

expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache #8105

expression,planner: support non-deterministic functions (e.g., now) in the prepared plan cache #8105

Conversation

dbjoa commented Oct 30, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

sre-bot commented Oct 30, 2018

dbjoa commented Oct 30, 2018

shenli commented Oct 30, 2018

eurekaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018 • edited Loading

CLAassistant commented Oct 31, 2018 • edited Loading

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

dbjoa commented Oct 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason left a comment

Choose a reason for hiding this comment

zz-jason commented Nov 1, 2018

dbjoa commented Oct 31, 2018 •

edited

Loading

CLAassistant commented Oct 31, 2018 •

edited

Loading