*: add builtin function TIMESTAMPDIFF #2386
Conversation
| sc := ctx.GetSessionVars().StmtCtx | ||
| t1, err := convertDatumToTime(sc, args[1]) | ||
| if err != nil { | ||
| return |
There was a problem hiding this comment.
No need, timestampdiff return NULL for illegal input.
mysql> select timestampdiff(MONTH, 'asdlfjasdf', '2017-01-01');
+--------------------------------------------------+
| timestampdiff(MONTH, 'asdlfjasdf', '2017-01-01') |
+--------------------------------------------------+
| NULL |
+--------------------------------------------------+
1 row in set, 1 warning (0.00 sec)
There was a problem hiding this comment.
May be it depends on sql_mode and statement type? Please try to insert timestampdiff(MONTH, 'asdlfjasdf', '2017-01-01') into some table.
| } | ||
| t2, err := convertDatumToTime(sc, args[2]) | ||
| if err != nil { | ||
| return |
| } | ||
| if t1.Time.Month() == 0 || t1.Time.Day() == 0 || | ||
| t2.Time.Month() == 0 || t2.Time.Day() == 0 { | ||
| return |
There was a problem hiding this comment.
Add a test case for this. MySQL perform in the same way?
There was a problem hiding this comment.
MySQL return NULL if month or day contains 0
Here is test case
https://github.com/pingcap/tidb/pull/2386/files#diff-9efa9861dfe0962aeeda87c63deb0f0fR559
Conflicts: expression/builtin_time_test.go plan/typeinferer_test.go
|
@tiancaiamao Fix conflicts. |
Conflicts: expression/builtin.go expression/builtin_time.go expression/builtin_time_test.go plan/typeinferer_test.go
| dayBeg = uint(t1.Day()) | ||
| dayEnd = uint(t2.Day()) | ||
| secondBeg = uint(t1.Hour()*3600 + t1.Minute()*60 + t1.Second()) | ||
| secondEnd = uint(t2.Hour()*3600 + t2.Minute()*60 + t2.Second()) |
There was a problem hiding this comment.
Can you add a function called totalSeconds to do this.
There was a problem hiding this comment.
hour * 3600 + minute * 60 + second is intuitive, totalSeconds doesn't make it better.
| case intervalDAY: | ||
| return int64(seconds) / secondsIn24Hour * negV | ||
| case intervalHOUR: | ||
| return int64(seconds) / 3600 * negV |
There was a problem hiding this comment.
Why not add a const called secondsInOneHour and secondsInOneMinute
There was a problem hiding this comment.
because 3600 is intuitive enough, and mysql also do that.
| // in microseconds fits into longlong. | ||
| return int64(seconds*1000000+microseconds) * negV | ||
| default: | ||
| break |
| } | ||
|
|
||
| // errorOrWarning reports error or warning depend on the context. | ||
| func errorOrWarning(err error, ctx context.Context) error { |
There was a problem hiding this comment.
Will this function be used in other place ? It's better to move it to util.go ?
There was a problem hiding this comment.
No other place use it currently, and we don't fully support warning now.
Maybe it would be use in the future, but we can refact then.
There was a problem hiding this comment.
Use StmtCtx instead of Context is better.
| if err != nil { | ||
| return d, errorOrWarning(err, ctx) | ||
| } | ||
| if t1.Time.Month() == 0 || t1.Time.Day() == 0 || t2.Time.Month() == 0 || t2.Time.Day() == 0 { |
There was a problem hiding this comment.
Do we need a function Time.Invalid() ?
|
LGTM |
| // TimestampDiff returns t2 - t1 where t1 and t2 are date or datetime expressions. | ||
| // The unit for the result (an integer) is given by the unit argument. | ||
| // The legal values for unit are "YEAR" "QUARTER" "MONTH" "DAY" "HOUR" "SECOND" and so on. | ||
| func TimestampDiff(unit string, t1 Time, t2 Time) int64 { |
There was a problem hiding this comment.
Because timestampDiff in mytime.go is not exported, expression package need use it.
I keep most mysql time function in mytime.go and keep them internal, while expose some of the for outside usage.
| } | ||
|
|
||
| // errorOrWarning reports error or warning depend on the context. | ||
| func errorOrWarning(err error, ctx context.Context) error { |
There was a problem hiding this comment.
Use StmtCtx instead of Context is better.
|
@tiancaiamao Please fix the conflict. |
Conflicts: expression/builtin_time.go
|
PTAL @shenli |
|
LGTM |
For issue #236