Fix evalEngine functions for dates on/before 0000-02-29
#15124
Conversation
beingnoble03
requested review from
harshit-gangal,
systay,
frouioui,
GuptaManan100 and
mattlord
as code owners
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15124 +/- ##
===========================================
+ Coverage 47.29% 70.64% +23.34%
===========================================
Files 1137 1376 +239
Lines 238684 182461 -56223
===========================================
+ Hits 112895 128902 +16007
+ Misses 117168 53559 -63609
+ Partials 8621 0 -8621 ☔ View full report in Codecov by Sentry. |
systay
added
Type: Enhancement
go/vt/vtgate/evalengine/fn_time.go
Outdated
| func convertTz(dt datetime.DateTime, from, to *time.Location, now time.Time) (datetime.DateTime, bool) { | ||
| t := dt.ToStdTime(now) | ||
| lowerBoundTz := time.Date(1970, 01, 01, 0, 0, 0, 0, time.UTC) | ||
| upperBoundTz := time.Date(3001, 1, 19, 3, 14, 7, 99999, time.UTC) |
There was a problem hiding this comment.
Is this based on https://dev.mysql.com/doc/dev/mysql-server/latest/my__time_8h.html#a67b86a393410bf1e9ab66ba46764c33d? It seems like it's slightly off from that value.
There was a problem hiding this comment.
We already have a const maxUnixtime = 32536771200 which represents this value anyway. So I think we should use that. We also should add a date this far in the future then to the tests I think to ensure the max value is also tested (and not just pre-unix epoch times).
There was a problem hiding this comment.
https://dev.mysql.com/doc/refman/8.3/en/date-and-time-functions.html#function_unix-timestamp
Says that for UNIX_TIMESTAMP is range is 1970-01-01 00:00:01.000000' UTC to '3001-01-19 03:14:07.999999' UTC (corresponding to 32536771199.999999 seconds).
[I checked, I think it should be 3001-01-18 23:59:59.999999 UTC, no idea why it says 3001-01-19 03:14:07.999999]
And,
https://dev.mysql.com/doc/refman/8.3/en/date-and-time-functions.html#function_convert-tz
Says that the range for CONVERT_TZ is '1970-01-01 00:00:01' UTC to '3001-01-18 23:59:59.999999' UTC.
There was a problem hiding this comment.
[I checked, I think it should be
3001-01-18 23:59:59.999999UTC, no idea why it says3001-01-19 03:14:07.999999]
Yeah, the docs are not always actually correct, the code speaks the truth usually 😄.
go/vt/vtgate/evalengine/fn_time.go
Outdated
| if t.Before(lowerBoundTz) || t.After(upperBoundTz) { | ||
| return dt, true | ||
| } | ||
|
|
||
| buf := datetime.DateTime_YYYY_MM_DD_hh_mm_ss.Format(dt, datetime.DefaultPrecision) | ||
| ts, err := time.ParseInLocation(time.DateTime, hack.String(buf), from) |
There was a problem hiding this comment.
We already parsed it into a Go Time here. So the above conversion seems wasteful to me. I think what we can do is after we've done this parse, is check against the unix timestamp instead? So something like this instead which seems a lot simpler?
diff --git i/go/vt/vtgate/evalengine/fn_time.go w/go/vt/vtgate/evalengine/fn_time.go
index ecb1fedc13..ac1f03b589 100644
--- i/go/vt/vtgate/evalengine/fn_time.go
+++ w/go/vt/vtgate/evalengine/fn_time.go
@@ -341,6 +341,9 @@ func convertTz(dt datetime.DateTime, from, to *time.Location) (datetime.DateTime
if err != nil {
return datetime.DateTime{}, false
}
+ if ts.Unix() < 0 || ts.Unix() >= maxUnixtime {
+ return dt, true
+ }
return datetime.NewDateTimeFromStd(ts.In(to)), true
}There was a problem hiding this comment.
Done. The implementation was wrong here in my changes, MySQL checks the limit after converting. Moved the check at the end.
| upperBoundTimestamp := time.Date(3001, 1, 19, 3, 14, 7, 99999, time.UTC) | ||
| if ts.Before(lowerBoundTimestamp) || ts.After(upperBoundTimestamp) { | ||
| return newEvalInt64(0) | ||
| } |
There was a problem hiding this comment.
Same here, I think testing against unix time stamps is much simpler here.
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
beingnoble03
force-pushed
the
fix-leap-year
branch
from
36ab579 to
cb27206
Compare
Signed-off-by: Noble Mittal <[email protected]>
Description
This PR fixes evalEngine functions for dates on/before
0000-02-29, along with fixes for zero time inevalToTime.Related Issue(s)
#15077
Checklist
Deployment Notes