evalengine: Implement SEC_TO_TIME
#15755
Conversation
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
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
beingnoble03
added
Type: Enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15755 +/- ##
==========================================
+ Coverage 68.38% 68.43% +0.04%
==========================================
Files 1556 1556
Lines 195361 195598 +237
==========================================
+ Hits 133598 133853 +255
+ Misses 61763 61745 -18 ☔ View full report in Codecov by Sentry. |
go/mysql/datetime/datetime.go
Outdated
| @@ -719,6 +719,52 @@ func NewTimeFromStd(t time.Time) Time { | |||
| } | |||
| } | |||
|
|
|||
| func NewTimeFromSecondsDecimal(seconds decimal.Decimal) Time { | |||
There was a problem hiding this comment.
Do we need the Decimal postfix on the function name here?
There was a problem hiding this comment.
we usually use float64 or int64 for representing seconds, so i thought it would be better to add Decimal prefix in the func name. altho i think it's already visible from the function signature, so removing it.
go/mysql/datetime/datetime.go
Outdated
| min := sec.Div(decimal.NewFromInt(60), 0) | ||
| _, sec = sec.QuoRem(decimal.NewFromInt(60), 0) | ||
|
|
||
| if h.Cmp(decimal.NewFromInt(839)) >= 0 { |
There was a problem hiding this comment.
Shall we extract 838 or 839 into a constant? It's also used in fn_time.go as well, so maybe time to do that? I'd probably say I have a tiny preference for checks in the style of > 838 vs. >= 839, but either works then really.
We can also use it in the next line then.
| if b.isHexOrBitLiteral() { | ||
| e = evalToDecimal(arg, 0, 0) | ||
| } else { | ||
| e = evalToDecimal(arg, 0, datetime.DefaultPrecision) |
There was a problem hiding this comment.
Is this the right precision? Or should it be inferred from the input string in this case?
There was a problem hiding this comment.
I think this should be the correct precision for this case, as mysql seems to set 6 as precision for *evalBytes (excluding Hex or Bit).
Signed-off-by: Noble Mittal <[email protected]>
go/mysql/datetime/datetime.go
Outdated
| h := sec.Div(decimal.NewFromInt(3600), 0) | ||
| _, sec = sec.QuoRem(decimal.NewFromInt(3600), 0) | ||
| min := sec.Div(decimal.NewFromInt(60), 0) | ||
| _, sec = sec.QuoRem(decimal.NewFromInt(60), 0) |
There was a problem hiding this comment.
Why are you doing a Div followed by QuoRem? The first returned element of QuoRem, which you're ignoring, is the quotient of the division. You can check the code to see that Div simply calls QuoRem under the hood. You're doing the same operation twice and throwing away the result!
There was a problem hiding this comment.
Also, we can move the decimal.NewFromInt to static global variables to make this method zero-allocation.
var (
decSecondsInHour = decimal.NewFromInt(3600)
decMinutesInHour = decimal.NewFromInt(60)
decMaxHours = decimal.NewFromInt(MaxHours)
)This way we get to reuse the decimal parsing and allocation between calls.
There was a problem hiding this comment.
got it, done. Thanks @vmg!
|
Looking good @beingnoble03! Let's make more efficient use of the decimal APIs before we merge this. |
Signed-off-by: Noble Mittal <[email protected]>
|
😎 |
Description
This PR adds implementation of
SEC_TO_TIMEfunc in evalengine.Related Issue(s)
Checklist
Deployment Notes