-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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(python): Convert date and datetime in literal construction #16018
Conversation
598ca60
to
22992c8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16018 +/- ##
=======================================
Coverage 80.72% 80.73%
=======================================
Files 1475 1475
Lines 193337 193351 +14
Branches 2760 2764 +4
=======================================
+ Hits 156079 156101 +22
+ Misses 36748 36740 -8
Partials 510 510 ☔ View full report in Codecov by Sentry. |
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.
Could you tweak the unit tests to include a wider range of dates/datetimes (including pre-epoch, etc)? 👍
@alexander-beedie thanks, working on parametrizing and indeed running into cases where we're outside the Datetime casting ability, but I'm not sure how to deal with it. It looks like from datetime import date
import polars as pl
pl.Series([date(1677, 9, 22)]).cast(pl.Datetime("ns")) # 1677-09-22 00:00:00
pl.Series([date(1677, 9, 21)]).cast(pl.Datetime("ns")) # 2262-04-11 23:34:33.709551616 |
22992c8
to
849292c
Compare
@alexander-beedie fixed up a few things and expanded the tests. Let me know if you think it needs more testing or changes. I restricted the times in the test to be within the Date -> Nanosecond allowance, as that can be addressed in the other issue I opened. |
849292c
to
8a8dd4e
Compare
Sorry, let this one sit; can you rebase/update and I'll take another look :) |
8a8dd4e
to
e717464
Compare
@alexander-beedie should be good once tests complete. |
Hey @alexander-beedie wanna merge this one before it goes stale again? |
Oops! Will look shortly 👍 |
6eb4bd5
to
7fe2e2d
Compare
@alexander-beedie rebased again. |
Hey @alexander-beedie this one's gone stale again and it's not too complex. |
Shame on me! 😅 |
7fe2e2d
to
5480c5a
Compare
Test failure due to #18211, unrelated to this PR. @alexander-beedie I think this is ready to go now. |
Fixes #16012.