builtin/time: support CURRENT_TIME(), CURTIME() #755

chenyanzhe · 2015-12-19T14:44:05Z

This is for issue #236. The change adds two time functions: CURRENT_TIME and CURTIME.

For github issue #236.

ngaut · 2015-12-19T14:57:55Z

Thanks @yanzhe-chen
Please add some tests.

chenyanzhe · 2015-12-19T15:56:43Z

@ngaut I am not sure how to test these two functions, since their results are run-time decided and accept no inputs. Could you give me some hints about how to do this?

ngaut · 2015-12-19T16:02:34Z

@yanzhe-chen For example, these code are just thoughts:
min := time.Now()
sqlNow := select CURTIME
assert(min < sqlNow < time.Now())

chenyanzhe · 2015-12-19T16:21:00Z

@ngaut Nice! I'll try that and update this request when done.

siddontang · 2015-12-20T01:03:14Z

mysql/time.go

-	DateFormat = "2006-01-02"
-	TimeFormat = "2006-01-02 15:04:05"
+	DateFormat    = "2006-01-02"
+	CurtimeFormat = "15:04:05"


CurTimeFormat or CurrentTimeFormat may be better.

@siddontang Fine, I will make that change.

chenyanzhe · 2015-12-20T03:16:05Z

@ngaut @siddontang I have added the missing test and changed the variable name as suggested. Please review the request again.

ngaut · 2015-12-20T04:35:49Z

Good, Please add some unit tests in parser/parser_test.go TestBuiltin.
https://github.com/pingcap/tidb/blob/master/parser/parser_test.go#L329

siddontang · 2015-12-20T05:41:35Z

expression/builtin/time.go

@@ -309,6 +309,26 @@ func builtinCurrentDate(args []interface{}, ctx map[interface{}]interface{}) (in
 		Type: mysql.TypeDate, Fsp: 0}, nil
 }

+// See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_curtime
+func builtinCurrentTime(args []interface{}, ctx map[interface{}]interface{}) (interface{}, error) {


Please use mysql.Duration instead of mysql.Time, or first you can use the mysql.Time to handle the argument, then convert it to mysql.Duration.

mysql.Time is not for duration, only for datetime, timestamp and date.

shenli · 2015-12-20T08:28:16Z

parser/scanner.l

@@ -692,6 +694,10 @@ year_month		{y}{e}{a}{r}_{m}{o}{n}{t}{h}
 			return curDate
 {current_date}		lval.item = string(l.val)
 			return currentDate
+{curtime}		lval.item = string(l.val)


curtime is not MySQL keyword and can be used as identifier. So it should be added into NotKeywordToken rule in parser.y. Those tokens can be used as Identifier.
https://github.com/pingcap/tidb/blob/master/parser/parser.y#L1685

current_time is a reserved keyword. So it can not be used as identifier. It is ok now.

… check

chenyanzhe · 2015-12-20T08:54:11Z

@ngaut I have added the corresponding tests in parser.

By the way, I forgot the the fsp arguments as stated in the MySQL doc so I added them as well.

@coocood I used mysql.Duration instead and added string length checks in test.

@shenli Thanks for reminding and I am trying to figure out how to fix that.

Please review it again and thanks for your patient help.

coocood · 2015-12-20T09:21:58Z

parser/parser.y

@@ -2137,6 +2139,22 @@ FunctionCallNonKeyword:
 	{
 		$$ = &ast.FuncCallExpr{FnName: model.NewCIStr($1.(string))}
 	}
+|	"CURTIME" FuncDatetimePrec


As "CURTIME" is synonym of "CURRENT_TIME", we can implement one of them, and parse the other one to the same type of token in scanner.l, like this PR:
#728

… at scanner part.

chenyanzhe · 2015-12-20T09:34:05Z

@coocood Thanks for your hint and I have made that change.

BTW I have found some other synonyms in code that are not handled in this way (e.g. curdate and current_date). Shall we create a new issue to fix these inconsistent implementations?

ngaut · 2015-12-20T09:59:53Z

Good catch! @yanzhe-chen
Please feel free to fire issues.

coocood · 2015-12-20T10:07:15Z

LGTM
@yanzhe-chen Good job!

siddontang · 2015-12-20T10:51:32Z

expression/builtin/builtin.go

 	"current_timestamp": {builtinNow, 0, 1, false, false},
+	"curtime":           {builtinCurrentTime, 0, 0, false, false},


the max arg should be 1. 0 means no argument.

The max arg should be 1, which means accept one argument.

chenyanzhe · 2015-12-20T11:34:57Z

@siddontang Thanks for catching that! Fix it.

chenyanzhe · 2015-12-20T14:02:05Z

expression/builtin/builtin.go

 	"current_timestamp": {builtinNow, 0, 1, false, false},
+	"curtime":           {builtinCurrentTime, 0, 1, false, false},
 	"date":              {builtinDate, 8, 8, true, false},


@siddontang Why builtinDate's minArgs and maxArgs are both set to 8? DATE should accept exactly one argument in the MySQL doc (DATE(expr)).
BTW, previously when I set current_time and curtime's maxArgs to 0, it can still pass all the tests. This makes me doubt whether this kind of checks really work.
Could you help me explain these? Thanks!

ngaut · 2015-12-20T14:29:19Z

Good catch, It's bug which describe here #758

chenyanzhe · 2015-12-20T15:27:43Z

@ngaut Nice, and I tried to fix this issue and fired a new pull request here. Please review it.

chenyanzhe · 2015-12-21T02:28:30Z

@siddontang @ngaut @shenli PTAL if there is anything to improve about this pull request or we can just merge and close this pull.

ngaut · 2015-12-21T02:31:45Z

LGTM, thanks @yanzhe-chen

builtin/time: support CURRENT_TIME(), CURTIME()

…ingcap#755)

) * add UnionStmt and ShowStmt as readonly * fix UnionStmt

* add UnionStmt and ShowStmt as readonly * fix UnionStmt

) * fix update keyspace config safe point version Signed-off-by: [email protected] <[email protected]> * fix update keyspace config safe point version Signed-off-by: [email protected] <[email protected]> * fix update keyspace config safe point version Signed-off-by: [email protected] <[email protected]> * fix update keyspace config safe point version Signed-off-by: [email protected] <[email protected]> --------- Signed-off-by: [email protected] <[email protected]>

builtin/time: support CURRENT_TIME(), CURTIME()

1b0c2e3

For github issue #236.

siddontang reviewed Dec 20, 2015
View reviewed changes

builtin/time: Add tests for CURRENT_TIME(), CURTIME()

2184577

builtin/time: Add tests in parser and support the fsp argument

8d0bf06

siddontang reviewed Dec 20, 2015
View reviewed changes

builtin/time: Add tests for the fsp argument in builtinCurrentTime()

8d938f3

shenli reviewed Dec 20, 2015
View reviewed changes

builtin/time: Use mysql.Duration instead of mysql.Time and add length…

7822670

… check

coocood reviewed Dec 20, 2015
View reviewed changes

builtin/time: Treat curTime as a synonym to currentTime and handle it…

1009e71

… at scanner part.

siddontang reviewed Dec 20, 2015
View reviewed changes

builtin/time: fix one bug in function mapping

0b70b41

The max arg should be 1, which means accept one argument.

chenyanzhe closed this Dec 20, 2015

chenyanzhe reopened this Dec 20, 2015

chenyanzhe reviewed Dec 20, 2015
View reviewed changes

ngaut added a commit that referenced this pull request Dec 21, 2015

Merge pull request #755 from yanzhe-chen/time-func

9bc1ea2

builtin/time: support CURRENT_TIME(), CURTIME()

ngaut merged commit 9bc1ea2 into pingcap:master Dec 21, 2015

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

YuJuncen pushed a commit to YuJuncen/tidb that referenced this pull request Apr 23, 2021

lightning/restore: fail fast when met error in engine/chunk restore (p…

d6efcd0

…ingcap#755)

xhebox pushed a commit to xhebox/tidb that referenced this pull request Sep 28, 2021

[parser] parser: set UnionStmt and ShowStmt as readonly (pingcap#755

82aef5a

) * add UnionStmt and ShowStmt as readonly * fix UnionStmt

xhebox pushed a commit to xhebox/tidb that referenced this pull request Oct 8, 2021

[parser] parser: set UnionStmt and ShowStmt as readonly (pingcap#755

f602d80

) * add UnionStmt and ShowStmt as readonly * fix UnionStmt

ti-chi-bot pushed a commit that referenced this pull request Oct 9, 2021

[parser] parser: set UnionStmt and ShowStmt as readonly (#755)

2f1915d

* add UnionStmt and ShowStmt as readonly * fix UnionStmt

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builtin/time: support CURRENT_TIME(), CURTIME() #755

builtin/time: support CURRENT_TIME(), CURTIME() #755

chenyanzhe commented Dec 19, 2015

ngaut commented Dec 19, 2015

chenyanzhe commented Dec 19, 2015

ngaut commented Dec 19, 2015

chenyanzhe commented Dec 19, 2015

siddontang Dec 20, 2015

chenyanzhe Dec 20, 2015

chenyanzhe commented Dec 20, 2015

ngaut commented Dec 20, 2015

siddontang Dec 20, 2015

shenli Dec 20, 2015

chenyanzhe commented Dec 20, 2015

coocood Dec 20, 2015

chenyanzhe commented Dec 20, 2015

ngaut commented Dec 20, 2015

coocood commented Dec 20, 2015

siddontang Dec 20, 2015

chenyanzhe commented Dec 20, 2015

chenyanzhe Dec 20, 2015

ngaut commented Dec 20, 2015

chenyanzhe commented Dec 20, 2015

chenyanzhe commented Dec 21, 2015

ngaut commented Dec 21, 2015

		"current_timestamp": {builtinNow, 0, 1, false, false},
		"curtime": {builtinCurrentTime, 0, 0, false, false},

builtin/time: support CURRENT_TIME(), CURTIME() #755

builtin/time: support CURRENT_TIME(), CURTIME() #755

Conversation

chenyanzhe commented Dec 19, 2015

ngaut commented Dec 19, 2015

chenyanzhe commented Dec 19, 2015

ngaut commented Dec 19, 2015

chenyanzhe commented Dec 19, 2015

siddontang Dec 20, 2015

Choose a reason for hiding this comment

chenyanzhe Dec 20, 2015

Choose a reason for hiding this comment

chenyanzhe commented Dec 20, 2015

ngaut commented Dec 20, 2015

siddontang Dec 20, 2015

Choose a reason for hiding this comment

shenli Dec 20, 2015

Choose a reason for hiding this comment

chenyanzhe commented Dec 20, 2015

coocood Dec 20, 2015

Choose a reason for hiding this comment

chenyanzhe commented Dec 20, 2015

ngaut commented Dec 20, 2015

coocood commented Dec 20, 2015

siddontang Dec 20, 2015

Choose a reason for hiding this comment

chenyanzhe commented Dec 20, 2015

chenyanzhe Dec 20, 2015

Choose a reason for hiding this comment

ngaut commented Dec 20, 2015

chenyanzhe commented Dec 20, 2015

chenyanzhe commented Dec 21, 2015

ngaut commented Dec 21, 2015