Skip to content
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.

Sign up for GitHub

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 SEC_TO_TIME #15755

Merged
merged 7 commits into from
Apr 23, 2024
Merged

evalengine: Implement SEC_TO_TIME #15755

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
99a072d
evalengine: Add initial implementation for SEC_TO_TIME
beingnoble03 Mar 31, 2024
beb06f2
evalengine:Handle time and decimal inputs for SEC_TO_TIME
beingnoble03 Apr 14, 2024
93bdb29
Add failing cases in SEC_TO_TIME
beingnoble03 Apr 15, 2024
86930d4
evalengine: Improve SEC_TO_TIME logic
beingnoble03 Apr 18, 2024
d62a741
Merge branch 'main' into eval-sec-to-time
beingnoble03 Apr 18, 2024
442c248
Rename NewTimeFromSeconds func and define datetime.MaxHours
beingnoble03 Apr 20, 2024
b9e929d
Make efficient use of decimal APIs
beingnoble03 Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions go/mysql/datetime/datetime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,6 +719,52 @@ func NewTimeFromStd(t time.Time) Time {
}
}

func NewTimeFromSecondsDecimal(seconds decimal.Decimal) Time {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the Decimal postfix on the function name here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var neg bool
if seconds.Cmp(decimal.NewFromInt(0)) < 0 {
neg = true
seconds = seconds.Mul(decimal.NewFromInt(-1))
}
beingnoble03 marked this conversation as resolved.
Show resolved Hide resolved

sec, frac := seconds.QuoRem(decimal.New(1, 0), 0)
ns := frac.Mul(decimal.New(1, 9))

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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, done. Thanks @vmg!


if h.Cmp(decimal.NewFromInt(839)) >= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

h := uint16(838)
if neg {
h |= negMask
}

return Time{
hour: h,
minute: 59,
second: 59,
nanosecond: 0,
}
}

hour, _ := h.Int64()
if neg {
hour |= int64(negMask)
}

m, _ := min.Int64()
s, _ := sec.Int64()
nsec, _ := ns.Int64()

return Time{
hour: uint16(hour),
minute: uint8(m),
second: uint8(s),
nanosecond: uint32(nsec),
}
}

func NewDateTimeFromStd(t time.Time) DateTime {
return DateTime{
Date: NewDateFromStd(t),
Expand Down
12 changes: 12 additions & 0 deletions go/vt/vtgate/evalengine/cached_size.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions go/vt/vtgate/evalengine/compiler_asm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4103,6 +4103,27 @@ func (asm *assembler) Fn_FROM_DAYS() {
}, "FN FROM_DAYS INT64(SP-1)")
}

func (asm *assembler) Fn_SEC_TO_TIME_D() {
asm.emit(func(env *ExpressionEnv) int {
e := env.vm.stack[env.vm.sp-1].(*evalTemporal)
prec := int(e.prec)

sec := newEvalDecimalWithPrec(e.toDecimal(), int32(prec))
env.vm.stack[env.vm.sp-1] = env.vm.arena.newEvalTime(datetime.NewTimeFromSecondsDecimal(sec.dec), prec)
return 1
}, "FN SEC_TO_TIME TEMPORAL(SP-1)")
}

func (asm *assembler) Fn_SEC_TO_TIME_d() {
asm.emit(func(env *ExpressionEnv) int {
e := env.vm.stack[env.vm.sp-1].(*evalDecimal)
prec := min(evalDecimalPrecision(e), datetime.DefaultPrecision)

env.vm.stack[env.vm.sp-1] = env.vm.arena.newEvalTime(datetime.NewTimeFromSecondsDecimal(e.dec), int(prec))
return 1
}, "FN SEC_TO_TIME DECIMAL(SP-1)")
}

func (asm *assembler) Fn_TIME_TO_SEC() {
asm.emit(func(env *ExpressionEnv) int {
if env.vm.stack[env.vm.sp-1] == nil {
Expand Down
69 changes: 69 additions & 0 deletions go/vt/vtgate/evalengine/fn_time.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ type (
CallExpr
}

builtinSecToTime struct {
CallExpr
}

builtinTimeToSec struct {
CallExpr
}
Expand Down Expand Up @@ -197,6 +201,7 @@ var _ IR = (*builtinMonthName)(nil)
var _ IR = (*builtinLastDay)(nil)
var _ IR = (*builtinToDays)(nil)
var _ IR = (*builtinFromDays)(nil)
var _ IR = (*builtinSecToTime)(nil)
var _ IR = (*builtinTimeToSec)(nil)
var _ IR = (*builtinToSeconds)(nil)
var _ IR = (*builtinQuarter)(nil)
Expand Down Expand Up @@ -1371,6 +1376,70 @@ func (call *builtinFromDays) compile(c *compiler) (ctype, error) {
return ctype{Type: sqltypes.Date, Flag: arg.Flag | flagNullable}, nil
}

func (b *builtinSecToTime) eval(env *ExpressionEnv) (eval, error) {
arg, err := b.arg1(env)
if arg == nil {
return nil, nil
}
if err != nil {
return nil, err
}

var e *evalDecimal
prec := datetime.DefaultPrecision

switch {
case sqltypes.IsDecimal(arg.SQLType()):
e = arg.(*evalDecimal)
case sqltypes.IsIntegral(arg.SQLType()):
e = evalToDecimal(arg, 0, 0)
case sqltypes.IsTextOrBinary(arg.SQLType()):
b := arg.(*evalBytes)
if b.isHexOrBitLiteral() {
e = evalToDecimal(arg, 0, 0)
} else {
e = evalToDecimal(arg, 0, datetime.DefaultPrecision)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the right precision? Or should it be inferred from the input string in this case?

Copy link
Member Author

@beingnoble03 beingnoble03 Apr 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}
case sqltypes.IsDateOrTime(arg.SQLType()):
d := arg.(*evalTemporal)
e = evalToDecimal(d, 0, int32(d.prec))
prec = int(d.prec)
default:
e = evalToDecimal(arg, 0, datetime.DefaultPrecision)
}

prec = min(int(evalDecimalPrecision(e)), prec)
return newEvalTime(datetime.NewTimeFromSecondsDecimal(e.dec), prec), nil
}

func (call *builtinSecToTime) compile(c *compiler) (ctype, error) {
arg, err := call.Arguments[0].compile(c)
if err != nil {
return ctype{}, err
}

skip := c.compileNullCheck1(arg)

switch {
case sqltypes.IsDecimal(arg.Type):
c.asm.Fn_SEC_TO_TIME_d()
case sqltypes.IsIntegral(arg.Type):
c.asm.Convert_xd(1, 0, 0)
c.asm.Fn_SEC_TO_TIME_d()
case sqltypes.IsTextOrBinary(arg.Type) && arg.isHexOrBitLiteral():
c.asm.Convert_xd(1, 0, 0)
c.asm.Fn_SEC_TO_TIME_d()
case sqltypes.IsDateOrTime(arg.Type):
c.asm.Fn_SEC_TO_TIME_D()
default:
c.asm.Convert_xd(1, 0, datetime.DefaultPrecision)
c.asm.Fn_SEC_TO_TIME_d()
}

c.asm.jumpDestination(skip)
return ctype{Type: sqltypes.Time, Flag: arg.Flag}, nil
}

func (b *builtinTimeToSec) eval(env *ExpressionEnv) (eval, error) {
arg, err := b.arg1(env)
if arg == nil {
Expand Down
16 changes: 16 additions & 0 deletions go/vt/vtgate/evalengine/testcases/cases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ var Cases = []TestCase{
{Run: FnLastDay},
{Run: FnToDays},
{Run: FnFromDays},
{Run: FnSecToTime},
{Run: FnTimeToSec},
{Run: FnToSeconds},
{Run: FnQuarter},
Expand Down Expand Up @@ -2060,6 +2061,21 @@ func FnFromDays(yield Query) {
}
}

func FnSecToTime(yield Query) {
for _, s := range inputConversions {
yield(fmt.Sprintf("SEC_TO_TIME(%s)", s), nil)
}

mysqlDocSamples := []string{
`SEC_TO_TIME(2378)`,
`SEC_TO_TIME(2378) + 0`,
}

for _, q := range mysqlDocSamples {
yield(q, nil)
}
}

func FnTimeToSec(yield Query) {
for _, d := range inputConversions {
yield(fmt.Sprintf("TIME_TO_SEC(%s)", d), nil)
Expand Down
5 changes: 5 additions & 0 deletions go/vt/vtgate/evalengine/translate_builtin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,11 @@ func (ast *astCompiler) translateFuncExpr(fn *sqlparser.FuncExpr) (IR, error) {
return nil, argError(method)
}
return &builtinFromDays{CallExpr: call}, nil
case "sec_to_time":
if len(args) != 1 {
return nil, argError(method)
}
return &builtinSecToTime{CallExpr: call}, nil
case "time_to_sec":
if len(args) != 1 {
return nil, argError(method)
Expand Down
Loading