evalengine: Implement LAST_DAY #15038

beingnoble03 · 2024-01-25T12:51:38Z

Description

This PR adds implementation of LAST_DAY func in evalengine.

Related Issue(s)

Fixes part of #9647

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

Deployment Notes

Signed-off-by: Noble Mittal <[email protected]>

vitess-bot · 2024-01-25T12:51:41Z

codecov · 2024-01-25T13:07:37Z

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (1bf8dda) 47.36% compared to head (c24efc8) 47.49%.
Report is 19 commits behind head on main.

Files	Patch %	Lines
go/vt/vtgate/evalengine/cached_size.go	0.00%	10 Missing ⚠️
go/vt/vtgate/evalengine/compiler_asm.go	71.42%	3 Missing and 1 partial ⚠️
go/vt/vtgate/evalengine/fn_time.go	92.50%	2 Missing and 1 partial ⚠️
go/vt/vtgate/evalengine/translate_builtin.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15038      +/-   ##
==========================================
+ Coverage   47.36%   47.49%   +0.13%     
==========================================
  Files        1144     1149       +5     
  Lines      238995   239387     +392     
==========================================
+ Hits       113197   113700     +503     
+ Misses     117213   117092     -121     
- Partials     8585     8595      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

systay · 2024-01-25T14:02:39Z

Hi @beingnoble03!

You need to run make codegen and then commit & push the changes.

That's why the static checks are failing

dbussink · 2024-01-25T14:02:51Z

go/vt/vtgate/evalengine/fn_time.go

@@ -1200,6 +1205,54 @@ func (call *builtinMonthName) compile(c *compiler) (ctype, error) {
 	return ctype{Type: sqltypes.VarChar, Col: col, Flag: arg.Flag | flagNullable}, nil
 }

+func lastDay(dt datetime.DateTime) (datetime.Date, bool) {
+	ts := dt.Date.ToStdTime(time.Local)
+	firstDayOfNextMonth := time.Date(ts.Year(), ts.Month()+1, 1, 0, 0, 0, 0, time.Local)


time.Local here should be the current timezone from the environment. Looks like that is wrong in more places, we should fix those all up then.

@beingnoble03 If you want, I can also push up fixes for the other spots that use time.Local, or you can do it, whatever you prefer really.

@dbussink I think I can do that. I'll push the changes.

dbussink · 2024-01-25T14:05:54Z

go/vt/vtgate/evalengine/fn_time.go

+func lastDay(dt datetime.DateTime) (datetime.Date, bool) {
+	ts := dt.Date.ToStdTime(time.Local)
+	firstDayOfNextMonth := time.Date(ts.Year(), ts.Month()+1, 1, 0, 0, 0, 0, time.Local)
+	lastDayOfMonth := firstDayOfNextMonth.AddDate(0, 0, -1)


I think we should do the addition with ts.AddDate(0, 1, -1) as well and not do that in time.Date (the ts.Month()+1 bit) to make it easier to follow and to avoid the question of "how does it work if the month is already December".

got it. done.

dbussink · 2024-01-25T14:06:46Z

go/vt/vtgate/evalengine/testcases/cases.go

+func FnLastDay(yield Query) {
+	for _, d := range inputConversions {
+		yield(fmt.Sprintf("LAST_DAY(%s)", d), nil)
+	}


Next to these inputs, we should also test it against a few more inputs explicitly to test edge case behaviors here. Like last of December, January & February years (with / without leap years etc).

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 · 2024-01-25T21:15:46Z

@systay thanks. done.

dbussink · 2024-01-25T21:22:22Z

go/vt/vtgate/evalengine/fn_time.go

@@ -756,7 +756,7 @@ func (call *builtinHour) compile(c *compiler) (ctype, error) {
 	return ctype{Type: sqltypes.Int64, Col: collationNumeric, Flag: arg.Flag | flagNullable}, nil
 }

-func yearDayToTime(y, yd int64) time.Time {
+func yearDayToTime(env *ExpressionEnv, y, yd int64) time.Time {


I think it’s best to pass in the most specific type. So we should pass in the time zone itself here I think.

Same applies to lastday as well.

Signed-off-by: Noble Mittal <[email protected]>

systay

nice work!

dbussink · 2024-01-26T10:19:29Z

go/vt/vtgate/evalengine/fn_time.go

+	}
+
+	d := lastDay(env.currentTimezone(), dt.dt)
+	if d.IsZero() {


Can this case happen? We already check above that it's not nil or a zero date, so how could it be a zero date here? I think we can remove this check (and the equivalent one from the compiler)?

hmm... true. This can not happen. Removed from here.
But I didn't add a check for zero datetime before calling lastDay in compiler_asm. And, there's no check in lastDay func for checking that, so added a check before calling lastDay and removed the check after that, as it was unnecessary.

Signed-off-by: Noble Mittal <[email protected]>

evalengine: Implement LAST_DAY

32a2831

Signed-off-by: Noble Mittal <[email protected]>

beingnoble03 requested review from harshit-gangal, systay, frouioui and GuptaManan100 as code owners January 25, 2024 12:51

github-actions bot added this to the v19.0.0 milestone Jan 25, 2024

dbussink reviewed Jan 25, 2024

View reviewed changes

Remove time.local, add edge cases for last_day func

d012b1c

Signed-off-by: Noble Mittal <[email protected]>

dbussink reviewed Jan 25, 2024

View reviewed changes

Pass time.Location instead of ExpressionEnv in lastDay and yearDayToTime

2ea4596

Signed-off-by: Noble Mittal <[email protected]>

systay approved these changes Jan 26, 2024

View reviewed changes

dbussink reviewed Jan 26, 2024

View reviewed changes

Add check for IsZero datetime in Fn_LAST_DAY

c24efc8

Signed-off-by: Noble Mittal <[email protected]>

dbussink approved these changes Jan 26, 2024

View reviewed changes

dbussink merged commit 44d6a6b into vitessio:main Jan 26, 2024
102 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

evalengine: Implement LAST_DAY #15038

evalengine: Implement LAST_DAY #15038

beingnoble03 commented Jan 25, 2024

vitess-bot bot commented Jan 25, 2024

codecov bot commented Jan 25, 2024 •

edited

Loading

systay commented Jan 25, 2024

dbussink Jan 25, 2024

dbussink Jan 25, 2024

beingnoble03 Jan 25, 2024

dbussink Jan 25, 2024 •

edited

Loading

beingnoble03 Jan 25, 2024

dbussink Jan 25, 2024

beingnoble03 Jan 25, 2024

beingnoble03 commented Jan 25, 2024

dbussink Jan 25, 2024

beingnoble03 Jan 25, 2024

systay left a comment

dbussink Jan 26, 2024

beingnoble03 Jan 26, 2024

evalengine: Implement LAST_DAY #15038

evalengine: Implement LAST_DAY #15038

Conversation

beingnoble03 commented Jan 25, 2024

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Jan 25, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Jan 25, 2024 • edited Loading

Codecov Report

systay commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbussink Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beingnoble03 commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

systay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 25, 2024 •

edited

Loading

dbussink Jan 25, 2024 •

edited

Loading