add ast evaluation for date arith #754

hhkbp2 · 2015-12-19T03:51:38Z

Hi there,

When I try to implement some time functions listed in issue #236, I find those functions adddate, subdate, date_add, date_sub, which are ticked as done, are not fully working. If they are used in interpreter, they just return NULL, as the below:

tidb> select adddate("2008-01-02", INTERVAL 1 DAY);
+---------------------------------------+
| adddate("2008-01-02", INTERVAL 1 DAY) |
+---------------------------------------+
| NULL                                  |
+---------------------------------------+
1 row in set (0.00 sec)
tidb> select adddate("2008-01-02", 1);
+--------------------------+
| adddate("2008-01-02", 1) |
+--------------------------+
| NULL                     |
+--------------------------+
1 row in set (0.00 sec)
tidb> select date_add("2008-01-02", INTERVAL 1 DAY);
+----------------------------------------+
| date_add("2008-01-02", INTERVAL 1 DAY) |
+----------------------------------------+
| NULL                                   |
+----------------------------------------+
1 row in set (0.00 sec)
tidb> select subdate("2008-01-02", 1);
+--------------------------+
| subdate("2008-01-02", 1) |
+--------------------------+
| NULL                     |
+--------------------------+
1 row in set (0.00 sec)

This PR add ast evalutions for date arith to fix this problem.

tidb> select adddate("2008-01-02", 1);
+---------------------------------------+
| adddate("2008-01-02", INTERVAL 1 DAY) |
+---------------------------------------+
| 2008-01-03                            |
+---------------------------------------+
1 row in set (0.00 sec)
tidb> select subdate("2008-01-02", INTERVAL 1 DAY);
+---------------------------------------+
| subdate("2008-01-02", INTERVAL 1 DAY) |
+---------------------------------------+
| 2008-01-01                            |
+---------------------------------------+
1 row in set (0.00 sec)
tidb> select date_add("2008-01-02", INTERVAL 1 DAY);
+----------------------------------------+
| date_add("2008-01-02", INTERVAL 1 DAY) |
+----------------------------------------+
| 2008-01-03                             |
+----------------------------------------+
1 row in set (0.00 sec)
tidb> select date_sub("2008-01-02", INTERVAL 1 DAY);
+----------------------------------------+
| date_sub("2008-01-02", INTERVAL 1 DAY) |
+----------------------------------------+
| 2008-01-01                             |
+----------------------------------------+
1 row in set (0.00 sec)

qiuyesuifeng · 2015-12-19T04:08:40Z

@hhkbp2 Thanks very much.
@coocood @zimulala PTAL

coocood · 2015-12-19T08:18:29Z

@hhkbp2 Thank you very much, please format the file you modified with 'go fmt`, otherwise, the Travis CI build would fail.

hhkbp2 · 2015-12-21T08:40:29Z

@qiuyesuifeng @coocood
Sorry for the malformation. I have reformatted these code and it passes checks now.

It's somewhat tricky to handle all the type castings and return type variations. I change a few test cases to adjust the behavior a little bit.
Please note, there are a few points where the behavior differs from mysql:

1. type cast from float string to interval

In mysql:

mysql> select adddate("2011-11-11 10:10:10", 19.88);
+---------------------------------------+
| adddate("2011-11-11 10:10:10", 19.88) |
+---------------------------------------+
| 2011-12-01 10:10:10                   |
+---------------------------------------+
1 row in set (0.00 sec)

mysql> select adddate("2011-11-11 10:10:10", "19.88");       
+-----------------------------------------+
| adddate("2011-11-11 10:10:10", "19.88") |
+-----------------------------------------+
| 2011-11-30 10:10:10                     |
+-----------------------------------------+
1 row in set, 1 warning (0.00 sec)

Since casting float to int will round the float value, but casting string to int (which contains a float value) will truncate the float value. I couldn't find a existing function in types package will do this exact conversion. So, in this PR, both cases hehave the same and return 2011-12-01 10:10:10.

2. loose conversion from string to int

In mysql, it supports a loose conversion from string to int, so we get:


mysql> select adddate("2011-11-11 10:10:10", "10,11"); 
+-----------------------------------------+
| adddate("2011-11-11 10:10:10", "10,11") |
+-----------------------------------------+
| 2011-11-21 10:10:10                     |
+-----------------------------------------+
1 row in set, 1 warning (0.00 sec)

mysql> select adddate("2011-11-11 10:10:10", "10-11");
+-----------------------------------------+
| adddate("2011-11-11 10:10:10", "10-11") |
+-----------------------------------------+
| 2011-11-21 10:10:10                     |
+-----------------------------------------+
1 row in set, 1 warning (0.00 sec)

In this PR, the above cases are reported as errors because types.Convert() doesn't support this kind of loose conversion.

siddontang · 2015-12-21T09:04:09Z

mysql/time.go

@@ -1375,3 +1375,40 @@ func ExtractTimeValue(unit string, format string) (int64, int64, int64, time.Dur
 		return 0, 0, 0, 0, errors.Errorf("invalid singel timeunit - %s", unit)
 	}
 }
+
+// IsHourMinuteSecondUnit returns true when unit is interval unit with hour, minutes or seconds.
+func IsHourMinuteSecondUnit(unit string) bool {


For go time package, it uses Date() for year, month, and day, uses Clock() for hour, minute and second, so I think here we can use IsClockUnit instead.

Agree. IsClockUnit() is a better name.

coocood · 2015-12-21T09:30:04Z

@hhkbp2 The existing test cases are copied from expression/date_arith_test.go and the function is implemented in expression/data_arith.go. The reason why this is not working right now is that evaluater.go didn't migrate the old code.

You can reference the old implement to make test cases pass with out changing it.

In TiDB, we enforce strict mode, so when data is truncated, it returns error instead of warning.

hhkbp2 · 2015-12-21T14:29:54Z

@coocood
Yes, I notice it mentions that strict mode on conversion is required in the source code, so loose conversion from string to int should be prohibited on purpose.

I don't quite understand the evolvement/history of copying code from expression/date_arith_test.go to evaluator_test.go. Could you help to elaborate it? It looks like that both expression/date_arith.go and evaluator.go have some codes works for similar functionality. Why duplicate?

coocood · 2015-12-21T14:55:46Z

@hhkbp2 The old expression, plan/plans, 'stmt/stmts' package has limited capability for optimization, and it can not be improved due to the design problem, but it supports all types of statements.
The new optimizer/plan, executor package has better support for optimization, but supports limited type of statements, we are supporting more statement on new plan, but for the statements we han't supported in new plan, we need to run it in the old way.
So there are a lot of duplicate code right now.
When new plan supports all the statements, old 'expression,plan/plans,stmt/stmts` package can be abandoned.

ngaut · 2015-12-24T04:09:33Z

@hhkbp2 Any updates?

hhkbp2 · 2015-12-24T09:19:38Z

@ngaut Referring to the code in expression/date_arith.go (thank @coocood for the above comments), I modify the code of this PR so that all the test cases remain still as expression/date_arith_test.go. All test cases go through OK now. And behaviors mentioned above keep the same with mysql, including the parsing of "19.88", "10,11", "10-11" for day interval.

ngaut · 2015-12-24T13:27:42Z

PTAL @coocood @siddontang

coocood · 2015-12-24T13:44:21Z

mysql/time.go

+func IsClockUnit(unit string) bool {
+	switch strings.ToUpper(unit) {
+	case "MICROSECOND", "SECOND", "MINUTE", "HOUR":
+		fallthrough


fallthrough is unnecessary, we can just list the rest of the cases in a new line.

Aha, right.

coocood · 2015-12-25T03:36:20Z

LGTM

zimulala · 2015-12-28T03:22:23Z

optimizer/evaluator/evaluator.go

+	if mysql.IsClockUnit(v.Unit) {
+		fieldType = mysql.TypeDatetime
+	}
+	resultField = types.NewFieldType(fieldType)


mybe resultField := types.NewFieldType(mysql.TypeDatetime) is enough. Don't need the code lines from line 866 to 886.

These lines of code 866 - 886 make the return data type of this function compatible with the counterpart in Mysql, which returns DATE or DATETIME due to the different date types of function arguments.

@hhkbp2 Actually, when we pass the argument to the function, the original type of the argument doesn't matter, we can force convert the argument to DATETIME, then do the calculation.

@coocood It seems to be a choice of how much compatible you would like to keep with Mysql.
I just followed the function spec in mysql docs. It would be OK to take the other force conversion way. It's simpler for both usage and implementation.

@hhkbp2 Converting the argument to DATETIME is the implementation detail, it makes the code cleaner.
If it doesn't give a different result, we can say it is compatible.

@coocood Precisely speaking, force conversion to DATETIME would return the same time value but different data type(and different output format in command line), which is not strictly consistent with the function spec in mysql docs.

@hhkbp2 Oh, you are right, but the test case didn't cover it, even if I force convert to DATETIME, the test still pass. Would you please add test case to cover it.
Thank you!

@coocood The corresponding test cases are added. And all test cases for date arith are re-organized to a better form.

coocood · 2015-12-29T04:10:53Z

LGTM
Well done!

zimulala · 2015-12-29T04:35:23Z

LGTM

add ast evaluation for date arith

* add Performance Schema Functions format_bytes and format_nano_time Signed-off-by: gauss <[email protected]> * remove a test case Signed-off-by: gauss <[email protected]> Co-authored-by: kennytm <[email protected]>

Signed-off-by: disksing <[email protected]> Co-authored-by: Yuqing Bai <[email protected]>

siddontang reviewed Dec 21, 2015
View reviewed changes

coocood reviewed Dec 24, 2015
View reviewed changes

zimulala reviewed Dec 28, 2015
View reviewed changes

add ast evaluation for date arith

405e6c5

zimulala added a commit that referenced this pull request Dec 29, 2015

Merge pull request #754 from hhkbp2/complete-adddate,subdate-functions

e45c31b

add ast evaluation for date arith

zimulala merged commit e45c31b into pingcap:master Dec 29, 2015

hhkbp2 deleted the complete-adddate,subdate-functions branch December 30, 2015 01:42

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

rleungx pushed a commit to rleungx/tidb that referenced this pull request Feb 26, 2024

vpa: tune gc parameters (pingcap#754)

504663d

Signed-off-by: disksing <[email protected]> Co-authored-by: Yuqing Bai <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ast evaluation for date arith #754

add ast evaluation for date arith #754

hhkbp2 commented Dec 19, 2015

qiuyesuifeng commented Dec 19, 2015

coocood commented Dec 19, 2015

hhkbp2 commented Dec 21, 2015

siddontang Dec 21, 2015

hhkbp2 Dec 21, 2015

coocood commented Dec 21, 2015

hhkbp2 commented Dec 21, 2015

coocood commented Dec 21, 2015

ngaut commented Dec 24, 2015

hhkbp2 commented Dec 24, 2015

ngaut commented Dec 24, 2015

coocood Dec 24, 2015

hhkbp2 Dec 25, 2015

coocood commented Dec 25, 2015

zimulala Dec 28, 2015

hhkbp2 Dec 28, 2015

coocood Dec 28, 2015

hhkbp2 Dec 28, 2015

coocood Dec 28, 2015

hhkbp2 Dec 28, 2015

coocood Dec 28, 2015

hhkbp2 Dec 29, 2015

coocood commented Dec 29, 2015

zimulala commented Dec 29, 2015

add ast evaluation for date arith #754

add ast evaluation for date arith #754

Conversation

hhkbp2 commented Dec 19, 2015

qiuyesuifeng commented Dec 19, 2015

coocood commented Dec 19, 2015

hhkbp2 commented Dec 21, 2015

1. type cast from float string to interval

2. loose conversion from string to int

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Dec 21, 2015

hhkbp2 commented Dec 21, 2015

coocood commented Dec 21, 2015

ngaut commented Dec 24, 2015

hhkbp2 commented Dec 24, 2015

ngaut commented Dec 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coocood commented Dec 25, 2015

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

coocood commented Dec 29, 2015

zimulala commented Dec 29, 2015