-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
evalengine: Implement TO_DAYS #15065
Conversation
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
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
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15065 +/- ##
==========================================
- Coverage 47.68% 47.64% -0.05%
==========================================
Files 1155 1151 -4
Lines 240064 239761 -303
==========================================
- Hits 114472 114228 -244
+ Misses 116990 116927 -63
- Partials 8602 8606 +4 ☔ View full report in Codecov by Sentry. |
`DATE'2023-09-03 07:00:00'`, | ||
`DATE'0000-00-00 00:00:00'`, | ||
`950501`, | ||
`'2007-10-07'`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://dev.mysql.com/doc/refman/8.0/en/date-and-time-functions.html#function_to-days has these cases:
TO_DAYS(950501);
TO_DAYS('2007-10-07');
TO_DAYS('2008-10-07'),
TO_DAYS('08-10-07');
TO_DAYS('0000-00-00');
TO_DAYS('0000-01-01');
Can you add the ones from there missing to make sure they all pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
go/vt/vtgate/evalengine/fn_time.go
Outdated
if date == nil { | ||
return nil, nil | ||
} | ||
dt := evalToDateTime(date, -1, env.now, env.sqlmode.AllowZeroDate()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have evalToDate
, should that be used here instead? The compiler uses Convert_xD
which convers to Date
as well (vs Convert_xDT
which is to DateTime
). We should be consistent between the compiler and evaluator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, used evalToDate
instead of evalToDateTime
Signed-off-by: Noble Mittal <[email protected]>
Signed-off-by: Noble Mittal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Could you please remove the redundant nil check?
@beingnoble03 Can you resolve the conflict here now that #15058 was merged? Fwiw, it's also fine for any (future) PR to implement more than 1 function. I've often implemented classes of related functions in one go before as well to reduce having many PRs in flight in parallel and having to deal with conflicts. |
Signed-off-by: Noble Mittal <[email protected]>
@dbussink done. Sure. I'll try to implement multiples functions in future PRs. |
Description
This PR adds implementation of
TO_DAYS
func in evalengine.Related Issue(s)
Fixes part of #9647
Checklist
Deployment Notes