Fix week number for date_format evalengine function#17432
Fix week number for date_format evalengine function#17432dbussink merged 3 commits intovitessio:mainfrom
Conversation
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
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
|
| func (fmtWeek1) format(dst []byte, t DateTime, prec uint8) []byte { | ||
| year, week := t.Date.ISOWeek() | ||
| if year < t.Date.Year() { | ||
| week = 0 | ||
| } | ||
| week := t.Date.Week(1) |
There was a problem hiding this comment.
I can make the same change for the other fmtWeek functions. Haven't done that yet, but i think it makes sense to just use the same implementation for everything.
There was a problem hiding this comment.
@GuptaManan100 i think we should do that indeed. Makes it less error prone and the week function afaik is already better tested too.
There was a problem hiding this comment.
@GuptaManan100 Pushed up the fix to always used the documented mode number.
|
I'm not sure about the backports either. Do we need to backport this fix to older releases? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17432 +/- ##
==========================================
- Coverage 67.68% 67.66% -0.02%
==========================================
Files 1583 1583
Lines 254321 254363 +42
==========================================
- Hits 172131 172122 -9
- Misses 82190 82241 +51 ☔ View full report in Codecov by Sentry. |
To ensure we avoid additional bugs, always use the mode documented instead if re-implementing something similar here. Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Don't have a super strong opinion, we could back port to v21 only as a small bugfix? |
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
…itessio#17432) (vitessio#17454) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Co-authored-by: Manan Gupta <manan@planetscale.com>
…ons (#768) * [release-19.0] Implement temporal comparisons (vitessio#17826) (vitessio#17852) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> * [release-19.0] Fix week number for date_format evalengine function (vitessio#17432) (vitessio#17454) Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Co-authored-by: Manan Gupta <manan@planetscale.com> --------- Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Co-authored-by: Manan Gupta <manan@planetscale.com>
Description
This PR fixes the way evalengine finds the week for the date_format function. It was noticed that the
Week()function works fine, but we weren't using it. This PR changes that to fix the issue ran into in #17431.Related Issue(s)
Checklist
Deployment Notes