expression: add builtinAddTimeSig #3290

hawkingrei · 2017-05-17T15:00:49Z

No description provided.

CLAassistant · 2017-05-17T15:00:56Z

All committers have signed the CLA.

shenli · 2017-05-17T15:21:45Z

@hawkingrei Thanks for your PR!
Please sign the CLA.

shenli

Please add unit test and modify plan/typeinferer.go.

shenli · 2017-05-17T15:22:07Z

expression/builtin_time.go

-	return d, errFunctionNotExists.GenByArgs("ADDTIME")
+	args, err := b.evalArgs(row)
+	if err != nil {
+		return types.Datum{}, errors.Trace(err)


return d, errors.Trace(err)

hanfei1991 · 2017-05-17T17:36:32Z

any test cases?

hawkingrei · 2017-05-18T02:06:40Z

I will complete the test case.

shenli · 2017-05-18T03:41:01Z

@hanfei1991 PTAL

hanfei1991 · 2017-05-18T03:45:10Z

expression/builtin_time.go

+		return d, errors.Trace(err)
+	}
+
+	duration := args[1].GetMysqlDuration()


you should convert it to duration

hawkingrei · 2017-05-19T03:15:15Z

Grean all

Have some problem.

ADDTIME(expr1,expr2)
1、 when expr2 expression is “1 1:1:1.000002”， it gives a error "invalid time format")” .it happen in
utils/time.go:343
2、 what is the type of result in addtime ?
expr1 (datetime) + expr2(duration) return datetime?
expr1 (duration) + expr2(duration) return duration?
3、 ADDTIME(expr1,expr2) only support that expr1 is datetime expression ,not time

…dd_builtinAddTimeSig

…rei/tidb into expression/add_builtinAddTimeSig

shenli · 2017-05-22T06:04:17Z

Is it the same behaviour with MySQL?

ADDTIME(expr1,expr2)
1、 when expr2 expression is “1 1:1:1.000002”， it gives a error "invalid time format")” .it happen in
utils/time.go:343

You can check MySQL's return type by using mysqlworkbench.

2、 what is the type of result in addtime ?
expr1 (datetime) + expr2(duration) return datetime?
expr1 (duration) + expr2(duration) return duration?

You can restrict the type of expr1.

3、 ADDTIME(expr1,expr2) only support that expr1 is datetime expression ,not time

XuHuaiyu · 2017-05-23T09:25:36Z

expression/builtin_time.go

@@ -1785,8 +1785,36 @@ type builtinAddTimeSig struct {

 // eval evals a builtinAddTimeSig.
 // See https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_addtime
+// ADDTIME(expr1,expr2)


remove line 1788,1789
it's useless which has been described in the URL.
and expr1 is a time or datetime expression, but expr2 is a time expression.

mysql> select addtime("12:00:01", 1);
+------------------------+
| addtime("12:00:01", 1) |
+------------------------+
| 12:00:02 |
+------------------------+

XuHuaiyu · 2017-05-23T10:18:51Z

expression/typeinferer.go

+		case mysql.TypeDatetime,mysql.TypeDate,mysql.TypeTimestamp:
+			tp = types.NewFieldType(mysql.TypeDatetime)
+		case mysql.TypeTime:
+			tp = types.NewFieldType(mysql.TypeVarString)


this should be mysql.TypeDuration

XuHuaiyu · 2017-05-23T10:20:46Z

expression/typeinferer.go

+	case ast.AddTime:
+		t := x.Args[0].GetType().Tp
+		switch t {
+		case mysql.TypeDatetime,mysql.TypeDate,mysql.TypeTimestamp:


args[0].Tp == mysql.TypeDate should be contained in default
and the chs should be setted to 'utf8' in the default case.

XuHuaiyu · 2017-05-23T10:27:46Z

expression/builtin_time_test.go

+	for _, t := range tbl {
+		tmpInput := types.NewStringDatum(t.Input)
+		tmpInputDuration := types.NewStringDatum(t.InputDuration)
+		//tmpResult := types.NewStringDatum(t.result)


Remove this line if it's useless.

XuHuaiyu · 2017-05-23T10:28:26Z

expression/typeinferer.go

@@ -367,6 +367,18 @@ func (v *typeInferrer) handleFuncCallExpr(x *ast.FuncCallExpr) {
 			tp.Charset, tp.Collate = types.DefaultCharsetForType(tp.Tp)
 		}
 	// number related


move this line to line 381

XuHuaiyu · 2017-05-23T10:29:53Z

expression/typeinferer_test.go

@@ -177,6 +177,8 @@ func (ts *testTypeInferrerSuite) TestInferType(c *C) {
 		{"weekday('2009-12-31 23:59:59.000010')", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag},
 		{"weekofyear('2009-12-31 23:59:59.000010')", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag},
 		{"yearweek('2009-12-31 23:59:59.000010')", mysql.TypeLonglong, charset.CharsetBin, mysql.BinaryFlag},
+		{"addtime('2007-12-31 23:59:59.999999', '1 1:1:1.000002')", mysql.TypeDatetime, charset.CharsetBin, mysql.BinaryFlag},


'2009-12-31 23:59:59.000010' is a string but not a datetime,
you can use c_datetime (which can be found in line51).

thx,i will complete this function as soon as possible

XuHuaiyu · 2017-05-23T10:30:55Z

expression/builtin_time_test.go

+		InputDuration string
+		expect        string
+	}{
+		//{"2007-12-31 23:59:59.999999", "1 1:1:1.000002", "2008-01-02 01:01:01.000001"},


which will TiDB get of these 4 cases?

XuHuaiyu · 2017-05-23T10:42:57Z

expression/builtin_time.go

+	if err != nil {
+		return d, errors.Trace(err)
+	}
+	arg1, err := types.ParseDuration(s, getFsp(s))


It seems the fsp of the result will always be types.MaxFsp if getFsp(s) >0;
and be types.MinFsp if getFsp(s) == 0

…rei/tidb into expression/add_builtinAddTimeSig

hawkingrei · 2017-06-01T09:08:50Z

@XuHuaiyu
Fixed all.

Sorry. I forget to push some fixed code.

add some invalid cases for “aa: bb: cc”
IsDuration checks the format of string whether is Duration or not. but it doesn't check the invalid. invalid checking will be done by parseDuration.

XuHuaiyu · 2017-06-01T09:15:38Z

expression/builtin_time.go

@@ -1770,6 +1770,72 @@ func getTimeZone(ctx context.Context) *time.Location {
 	return ret
 }

+// isDuration returns a boolean indicating whether the str is the format of duration.


s/is the format/matches the format

add comment for the format that str should match.

@XuHuaiyu Use regular expression install of the old way.

XuHuaiyu · 2017-06-01T09:18:30Z

expression/builtin_time_test.go

+		c.Assert(result, Equals, t.expect)
+	}
+
+	result := isDuration("aa:bb:cc")


I think we don't need the comparison of these two cases here.

XuHuaiyu · 2017-06-01T09:22:44Z

PTAL
@coocood @tiancaiamao

…rei/tidb into expression/add_builtinAddTimeSig

tiancaiamao · 2017-06-02T08:11:02Z

expression/builtin_time.go

@@ -1770,6 +1770,52 @@ func getTimeZone(ctx context.Context) *time.Location {
 	return ret
 }

+// isDuration returns a boolean indicating whether the str is the format of duration.
+func isDuration(str string) bool {
+	const durationArgReg = `^(|[-]?)(|\d{1,2}\s)(\d{2,3}:\d{2}:\d{2}|\d{1,2}:\d{2}|\d{1,6})(|\.\d*)$`


You can pre-compile the regexp and store it as a global variable, rather than compile it every time.
Or maybe it's a non-frequency function and you can just leave as it is.

And add comment for it, better with a example: "xx:xx:xx", otherwise, nobody can understand this regexp.

@tiancaiamao Fix

tiancaiamao · 2017-06-02T08:13:29Z

expression/builtin_time.go

+	return r.MatchString(str)
+}
+
+// strDatetimeAddDuration adds duration to datetime string, returns a dutam value.


s/dutam/datum

@tiancaiamao Fix

tiancaiamao · 2017-06-02T08:16:23Z

expression/builtin_time.go

+		return result, errors.Trace(err)
+	}
+	if getFsp(d) != 0 {
+		tmpDuration.Fsp = types.MaxFsp


Is this MySQL's behavior ?
By the way, you can fsp := getFsp(d) to avoid call getFsp again and again.

@tiancaiamao
Yes
mysql> select addtime("1:23","1.98");
+------------------------+
| addtime("1:23","1.98") |
+------------------------+
| 01:23:01.980000 |
+------------------------+
1 row in set (0.00 sec)

mysql> select addtime("123","1:23.98");
+--------------------------+
| addtime("123","1:23.98") |
+--------------------------+
| 01:24:23.980000 |
+--------------------------+
1 row in set (0.00 sec)

tiancaiamao · 2017-06-02T08:18:39Z

expression/builtin_time.go

+	if err != nil {
+		return d, errors.Trace(err)
+	}
+	if args[0].IsNull() || args[1].IsNull() {


Need to check len(args) == 2 , or is it already done in type infer?

@tiancaiamao it is already done in buildin.go

…rei/tidb into expression/add_builtinAddTimeSig

hawkingrei · 2017-06-02T10:10:56Z

@tiancaiamao @XuHuaiyu @hanfei1991 @shenli
PTAL

tiancaiamao · 2017-06-05T03:57:17Z

LGTM @XuHuaiyu

XuHuaiyu · 2017-06-05T04:00:56Z

expression/typeinferer.go

@@ -367,6 +367,18 @@ func (v *typeInferrer) handleFuncCallExpr(x *ast.FuncCallExpr) {
 			tp.Charset, tp.Collate = types.DefaultCharsetForType(tp.Tp)
 		}
 	// number related


this comment should be in line 381. @hawkingrei

…rei/tidb into expression/add_builtinAddTimeSig

hawkingrei · 2017-06-05T07:07:33Z

@XuHuaiyu Fix it

XuHuaiyu · 2017-06-05T07:10:59Z

LGTM

expression: add builtinAddTimeSig

1e1577b

shenli reviewed May 17, 2017

View reviewed changes

hanfei1991 reviewed May 18, 2017

View reviewed changes

真人 added 4 commits May 19, 2017 00:02

expression: add builtinAddTimeSig

d34c67f

expression: test AddTime

19d4e59

expression: add ast.Addtime type inferer

9219456

expression: test ast.Addtime type inferer

9d93b14

hawkingrei and others added 5 commits May 19, 2017 11:35

Merge branch 'master' into expression/add_builtinAddTimeSig

c7b5d58

Merge branch 'master' of github.com:hawkingrei/tidb into expression/a…

09b878e

…dd_builtinAddTimeSig

typeinferer

7fff1b2

Merge branch 'master' into expression/add_builtinAddTimeSig

decec95

Merge branch 'expression/add_builtinAddTimeSig' of github.com:hawking…

0c32cee

…rei/tidb into expression/add_builtinAddTimeSig

zimulala added the contribution This PR is from a community contributor. label May 22, 2017

feat: reconstruction typeinferer

c7a9f2f

Merge branch 'master' into expression/add_builtinAddTimeSig

667ea29

XuHuaiyu reviewed May 23, 2017

View reviewed changes

真人 and others added 7 commits May 23, 2017 19:52

fix typeinferer

4d06c9e

Merge branch 'master' into expression/add_builtinAddTimeSig

2d8e49b

fix: addtime build fail

2bf974c

go fmt

8722d12

fix: XuHuaiyu say move this line to line 381

81c697a

go fmt

1da9735

feat: complete part

157f8d2

真人 added 3 commits June 1, 2017 17:03

fix all

a3ea727

Merge branch 'expression/add_builtinAddTimeSig' of github.com:hawking…

b3598d0

…rei/tidb into expression/add_builtinAddTimeSig

change commit

cd638d5

Merge branch 'master' into expression/add_builtinAddTimeSig

cda3ceb

XuHuaiyu reviewed Jun 1, 2017

View reviewed changes

真人 and others added 5 commits June 1, 2017 17:35

remove useless test case

099013e

Merge branch 'expression/add_builtinAddTimeSig' of github.com:hawking…

5a77b4b

…rei/tidb into expression/add_builtinAddTimeSig

Merge branch 'master' into expression/add_builtinAddTimeSig

3db1c61

use regular expression

2256ea2

Merge branch 'master' into expression/add_builtinAddTimeSig

c0ca76a

tiancaiamao reviewed Jun 2, 2017

View reviewed changes

真人 added 3 commits June 2, 2017 17:36

fix all

c606c7c

Merge branch 'expression/add_builtinAddTimeSig' of github.com:hawking…

0f1edcd

…rei/tidb into expression/add_builtinAddTimeSig

fix style

04b1da3

Merge branch 'master' into expression/add_builtinAddTimeSig

343a3ce

tiancaiamao added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 5, 2017

XuHuaiyu reviewed Jun 5, 2017

View reviewed changes

真人 added 2 commits June 5, 2017 14:57

fix typeinferer commit

35709c5

Merge branch 'expression/add_builtinAddTimeSig' of github.com:hawking…

6812460

…rei/tidb into expression/add_builtinAddTimeSig

Merge branch 'master' into expression/add_builtinAddTimeSig

b743c33

XuHuaiyu approved these changes Jun 5, 2017

View reviewed changes

XuHuaiyu added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 5, 2017

XuHuaiyu merged commit e669009 into pingcap:master Jun 5, 2017

expression: add builtinAddTimeSig #3290

expression: add builtinAddTimeSig #3290

Conversation

hawkingrei commented May 17, 2017

CLAassistant commented May 17, 2017 • edited

shenli commented May 17, 2017

shenli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanfei1991 commented May 17, 2017

hawkingrei commented May 18, 2017

shenli commented May 18, 2017

Choose a reason for hiding this comment

hawkingrei commented May 19, 2017

shenli commented May 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkingrei commented Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuHuaiyu commented Jun 1, 2017

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

hawkingrei Jun 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkingrei Jun 2, 2017 • edited

Choose a reason for hiding this comment

hawkingrei commented Jun 2, 2017 • edited

tiancaiamao commented Jun 5, 2017

Choose a reason for hiding this comment

hawkingrei commented Jun 5, 2017

XuHuaiyu commented Jun 5, 2017

CLAassistant commented May 17, 2017 •

edited

hawkingrei commented Jun 1, 2017 •

edited

hawkingrei Jun 2, 2017 •

edited

hawkingrei Jun 2, 2017 •

edited

hawkingrei commented Jun 2, 2017 •

edited