Skip to content

Conversation

@tswast
Copy link
Collaborator

@tswast tswast commented Dec 3, 2021

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

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

@tswast tswast requested a review from a team December 3, 2021 21:48
@tswast tswast requested a review from a team as a code owner December 3, 2021 21:48
@tswast tswast requested a review from loferris December 3, 2021 21:48
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 3, 2021
@product-auto-label product-auto-label bot added the api: bigquery Issues related to the googleapis/python-db-dtypes-pandas API. label Dec 3, 2021
],
type=pyarrow.time64("ns"),
),
id="time-nanoseconds-arrow-round-trip",
Copy link
Collaborator Author

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.

Copy link

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()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Towards #19

Copy link
Collaborator Author

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.

@tswast tswast requested a review from plamut December 3, 2021 23:38
Copy link

@plamut plamut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

],
type=pyarrow.time64("ns"),
),
id="time-nanoseconds-arrow-round-trip",
Copy link

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. 👍

@tswast tswast merged commit 4253358 into main Dec 4, 2021
@tswast tswast deleted the issue45-extreme-dates branch December 4, 2021 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: bigquery Issues related to the googleapis/python-db-dtypes-pandas API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: loading DATE columns with extreme values results in incorrect data (pyarrow.lib.ArrowInvalid casting error in v3)

2 participants