-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: let "ambiguous" take "null" value #14961
Conversation
3329376
to
c520fd1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14961 +/- ##
=======================================
Coverage 80.99% 80.99%
=======================================
Files 1333 1333
Lines 173174 173229 +55
Branches 2458 2458
=======================================
+ Hits 140265 140312 +47
- Misses 32442 32449 +7
- Partials 467 468 +1 ☔ View full report in Codecov by Sentry. |
80b07d8
to
00c0d71
Compare
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.
I'll leave the Rust review to someone else, but I'm happy with the addition of the "null"
option!
39fe5e4
to
1b1069f
Compare
37cfb09
to
d15e021
Compare
d15e021
to
2377d26
Compare
match="datetime '2021-03-28 02:30:00' is non-existent in time zone 'Europe/Warsaw'", | ||
): | ||
pl.Series(["2021-03-28 02:30"]).str.to_datetime( | ||
time_unit="us", | ||
time_zone="Europe/Warsaw", | ||
ambiguous="null", | ||
) |
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.
separately, a non_existent
argument could be added to make sure that this would return null
values instead of raising. This is just to check that ambiguous='null'
doesn't affect what happens for non-existent datetimes (coverage purposes)
EDIT: #15062 opens the doors for non_existent
to be rebased onto #14958Quick demo of what this lets you do:
towards #11579. Based on #14842 (comment), I think there's a need for being able to just have
null
for problematic datetimes.I'm suggesting to start with adding
ambiguous='null'
, and then adding the optionnon_existent: Literal['null', 'raise']
Perf implication:
ambiguous='raise'
(the default), no implication. There's already a fast-path for thatambiguous='raise'
, the default): no impact.ambiguous
: no impact. I've made a fastpath to preserve this case. Check there's no impact here, which shows 12.373298853000051 => 12.362355302333375 (main => here)PolarsResult<Option<i64>>
as opposed toPolarsResult<i64>
, but I think it's worth it for the user benefit