-
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 TIME_TO_SEC #15094
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
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15094 +/- ##
==========================================
+ Coverage 47.70% 47.79% +0.09%
==========================================
Files 1155 1155
Lines 240218 240418 +200
==========================================
+ Hits 114593 114906 +313
+ Misses 117020 116940 -80
+ Partials 8605 8572 -33 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Noble Mittal <[email protected]>
} | ||
d := env.vm.stack[env.vm.sp-1].(*evalTemporal) | ||
|
||
sec := d.dt.Time.Hour()*3600 + d.dt.Time.Minute()*60 + d.dt.Time.Second() |
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 logic is duplicated in the evaluation VM too. Could you please extract it into a ToSeconds()
method in Time
?
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.
Signed-off-by: Noble Mittal <[email protected]>
@vmg can you please review? |
Thank you! |
Description
This PR adds implementation of
TIME_TO_SEC
func in evalengine.Related Issue(s)
Fixes part of #9647
Checklist
Deployment Notes