-
-
Notifications
You must be signed in to change notification settings - Fork 17.8k
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.
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
BUG: Fix near-minimum timestamp handling #57314
BUG: Fix near-minimum timestamp handling #57314
Conversation
@WillAyd where would I add tests for this? If I'm not mistaken, then the |
I'll fix the failing Windows tests and found in the documentation some guidance on where and how to add the tests. Will mark ready for review once done. |
Heads up - I won't be able to review in detail until early next week |
f078a24
to
d0675f4
Compare
I have started over with a failing test, see the results on b4c825d. To resolve, I went with a minimally invasive approach that just checks for negative extreme before scaling to microseconds, and then doing the calculation backwards from the minimum valid timestamp. I didn't find any constant defined for this ( I only fixed the nanosecond case, because I was unable to devise tests for any other unit that would trigger this overflow, because:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool this looks pretty good. Thanks for the great work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm @mroeschke
Thanks @robert-schmidtke |
* attempt failing test * expand test for demonstration purposes * fix near-minimum timestamp overflow when scaling from microseconds to nanoseconds * minor refactor * add comments around specifically handling near-minimum microsecond and nanosecond timestamps * consolidate comments --------- Co-authored-by: Robert Schmidtke <robert.schmidtke@trailstonegroup.com>
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Previously, timestamps
1677-09-21 00:12:43.145224193
thru1677-09-21 00:12:43.145224999
would raise anOverflowError
due to the fact thatscaleMicrosecondsToNanoseconds
overflows as an intermediate step. After adding back the nanoseconds part, the timestamp would be valid again.