-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: raise ValueError if date is out-of-bounds #46
Conversation
@@ -216,6 +216,7 @@ def types_mapper( | |||
], | |||
type=pyarrow.time64("ns"), | |||
), | |||
id="time-nanoseconds-arrow-round-trip", |
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.
Leftover from debugging why when I made the "time" _datetime
method return a Timestamp
too, this broke. Since that change isn't necessary and it broke this test I reverted it. Kept the test id, since I thought it was useful.
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.
Test ID is indeed useful IMO. 👍
minute=int(minute) if minute else 0, | ||
second=int(second) if second else 0, | ||
nanosecond=nanosecond, | ||
).to_datetime64() |
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.
Towards #19
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.
Probably should have been a separate PR, but I noticed _datetime
had inconsistent types (sometimes pandas.Timestamp, sometime datetime.datetime, sometime numpy.datetime64). We want a numpy.datetime64
in the end, so I unified it.
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
@@ -216,6 +216,7 @@ def types_mapper( | |||
], | |||
type=pyarrow.time64("ns"), | |||
), | |||
id="time-nanoseconds-arrow-round-trip", |
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.
Test ID is indeed useful IMO. 👍
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #45 🦕
Edit: Maybe should be just "Towards" #45 because I think we still need some way to support out-of-range values so that I can load DATE values when using the conversion logic googleapis/python-bigquery-pandas#441