expression: keep month and day when a year/month interval underflows to year 0#70004
expression: keep month and day when a year/month interval underflows to year 0#70004anshgoyalevil wants to merge 2 commits into
Conversation
…to year 0
DATE_SUB('1000-01-01', INTERVAL 1000 YEAR) returned 0000-00-00 instead of
0000-01-01. addDate cleared the month and day whenever the result year hit 0,
which is only correct for day/time intervals that underflow past the minimum
datetime (issue pingcap#11329). Year and month intervals do calendar arithmetic, so
MySQL keeps the month and day. Gate the clearing on day == 0 && nano == 0 so
only day/time intervals zero them out.
Issue Number: close pingcap#59789
Signed-off-by: Miles Porter <anshgoyal1704@gmail.com>
ti-chi-bot
Bot
added
release-note
|
Hi @anshgoyalevil. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
size/S
|
Welcome @anshgoyalevil! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDate arithmetic now preserves the computed month and day when subtracting years reaches year zero. A regression test covers subtracting 1000 years from ChangesDate arithmetic correction
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/expression/builtin_time_test.go (1)
2399-2401: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a MONTH-underflow regression case.
The changed production branch is shared by YEAR and MONTH intervals, but this test only exercises YEAR. Add a table entry that crosses year zero with a MONTH interval and verifies that the computed month and day are preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/expression/builtin_time_test.go` around lines 2399 - 2401, Add a regression table entry alongside the existing YEAR case in the relevant time-interval tests, using a MONTH interval that crosses year zero and asserting the resulting month and day remain unchanged. Keep the case focused on the shared production branch and match the existing test table’s input, interval, and expected-value format.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/expression/builtin_time.go`:
- Around line 3207-3217: Update the branch in the date arithmetic logic around
SetCoreTime so zero-valued day/time intervals are not treated as calendar
intervals: require year or month to be nonzero in addition to day == 0 and nano
== 0 before preserving goTime’s month/day. Keep the existing zero-date
normalization for INTERVAL 0 DAY, 0 HOUR, and other intervals without year/month
components.
---
Nitpick comments:
In `@pkg/expression/builtin_time_test.go`:
- Around line 2399-2401: Add a regression table entry alongside the existing
YEAR case in the relevant time-interval tests, using a MONTH interval that
crosses year zero and asserting the resulting month and day remain unchanged.
Keep the case focused on the shared production branch and match the existing
test table’s input, interval, and expected-value format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d046b541-02e6-4fb5-8998-8ba3a6b5e9a5
📒 Files selected for processing (2)
pkg/expression/builtin_time.gopkg/expression/builtin_time_test.go
… fields Guard the year-0 calendar branch with year != 0 || month != 0 so a zero interval (INTERVAL 0 DAY / 0 HOUR) on a year-0 date keeps the existing zero-date normalization instead of being treated as a calendar interval. Signed-off-by: Miles Porter <anshgoyal1704@gmail.com>
1 participant
What problem does this PR solve?
Issue Number: close #59789
Problem Summary:
DATE_SUB('1000-01-01', INTERVAL 1000 YEAR)returned0000-00-00, while MySQL returns0000-01-01. When an interval drives the resulting year down to 0,addDatecleared the month and day unconditionally.What changed and how does it work?
The year-0 handling was added for day/time intervals that underflow past the minimum datetime, where MySQL does return a zero date and keeps the wrapped time (e.g.
DATE_ADD('0001-01-01 00:00:00', INTERVAL -2 HOUR)->0000-00-00 22:00:00, issue #11329). Year/month intervals instead do calendar arithmetic, so MySQL preserves the month and day.ParseDurationValueyieldsday == 0 && nano == 0only for YEAR/MONTH/QUARTER intervals; DAY/WEEK and all time units setdayornano. Gating the clearing onday == 0 && nano == 0keeps the month/day for calendar intervals and leaves the existing day/time underflow behaviour untouched.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
1000-01-01with a-1000year interval.