-
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 FROM_DAYS #15058
Conversation
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
|
@beingnoble03 We also already have https://github.com/vitessio/vitess/blob/main/go/mysql/datetime/mydate.go#L49-L83 which is used in a number of cases, so maybe that's what we need to use here too? |
@dbussink true. So, should we write a func |
I think we can rename it to export it so we can use it from the evalengine. |
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]>
go/vt/vtgate/evalengine/fn_time.go
Outdated
return nil, nil | ||
} | ||
|
||
dt := datetime.NewDateFromStd(time.Date(int(y), time.Month(m), int(d), 0, 0, 0, 0, env.currentTimezone())) |
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.
This conversion seems unneeded? I think you can just straight set the properties from what MysqlDateFromDayNumber
returns?
var dt datetime.Date
dt.Year, dt.Month, dt.Day = mysqldt.MysqlDateFromDayNumber(int(arg.i))
Then you also only have to check dt.Year > 9999
, the zero case also automatically falls out?
Same for the compiler, no need for the conversion there either through Go's Time library.
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.
The struct fields of datetime.Date
are not exported. So, I think we can't follow this.
Also, don't know why I imported mysql/datetime
twice. Fixing it.
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.
Instead of exporting the existing function, we can also add a new one that returns the date already so you don’t have that issue.
I think we really should avoid going through the go type if not strictly needed here.
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.
got it. done.
Fixing the failing tests. |
Signed-off-by: Noble Mittal <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15058 +/- ##
==========================================
+ Coverage 47.49% 47.51% +0.01%
==========================================
Files 1149 1149
Lines 239475 239532 +57
==========================================
+ Hits 113730 113804 +74
+ Misses 117138 117122 -16
+ Partials 8607 8606 -1 ☔ View full report in Codecov by Sentry. |
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.
Nice work! Is TO_DAYS
next to keep it symmetric 😉?
@dbussink sure! |
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! Thanks for contributing to Vitess!
Description
This PR adds implementation of
FROM_DAYS
func in evalengine.Related Issue(s)
Fixes part of #9647
Checklist
Deployment Notes