Fix DateTime add/subtract of weeks: 52 weeks is 364 days, not a calendar year#1790
Open
c-schuler wants to merge 1 commit into
Open
Fix DateTime add/subtract of weeks: 52 weeks is 364 days, not a calendar year#1790c-schuler wants to merge 1 commit into
c-schuler wants to merge 1 commit into
Conversation
Related IssuesThe following open issues may be related to this PR:
|
|
Formatting check succeeded! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DateTime/Datearithmetic drifted by a day whenever a quantity of 52 or more weeks was added or subtracted. For example:DateTime(2018, 5, 23) + 52 weeks // returned 2019-05-23, should be 2019-05-2252 weeks is a fixed 364 days (and should preserve the day of week - Wed -> Wed), but the engine was returning a result 365 days later (the calendar anniversary).
Root cause
TemporalHelper.weeksToDays special-cased blocks of 52 weeks as 365-day calendar years:
This is correct for weeks < 52 (weeks * 7) but jumps by an extra day at every 52-week boundary (51 weeks → 357 days, 52 weeks → 365). A week is a fixed 7-day duration, so the year rollup has no basis in the spec.
Fix
fun weeksToDays(weeks: Int): Int = weeks * 7
Weeks are converted to days and added to the day component, letting the existing date logic walk the calendar (so leap-day/leap-year cases land correctly).
This matches the normative spec - CQL Author's Guide, Date/Time Arithmetic:
(and the reference's "1 week = 7 days"), rather than any "52 weeks ~ 1 year" assumption.
Validation
Scope
Fixes the add/subtract weeks path only. The "difference in weeks" issue (#1788 separate operator (DifferenceBetween) and is not addressed here.
Closes #1726, #1765