-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(rust): Make casting to date
timezone-aware
#22888
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
base: main
Are you sure you want to change the base?
fix(rust): Make casting to date
timezone-aware
#22888
Conversation
py-polars/tests/unit/operations/namespaces/temporal/test_datetime.py
Outdated
Show resolved
Hide resolved
thanks for your pr! just left a couple of comments |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #22888 +/- ##
==========================================
- Coverage 81.01% 80.62% -0.40%
==========================================
Files 1671 1677 +6
Lines 236927 222321 -14606
Branches 2792 2801 +9
==========================================
- Hits 191949 179241 -12708
+ Misses 44309 42412 -1897
+ Partials 669 668 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
py-polars/tests/unit/operations/namespaces/temporal/test_datetime.py
Outdated
Show resolved
Hide resolved
let from_tz = tz.to_chrono()?; | ||
let ambiguous = StringChunked::from_iter(std::iter::once("raise")); | ||
self.phys.apply_values(|timestamp| { | ||
let ndt = timestamp_to_datetime(timestamp); | ||
let res = convert_to_naive_local( | ||
&from_tz, | ||
&Tz::UTC, | ||
ndt, | ||
Ambiguous::from_str(ambiguous.get(0).unwrap()).unwrap(), | ||
NonExistent::Raise, | ||
) | ||
.unwrap(); |
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.
thanks for updating
i think there's a function that you can use to do this whole part (replace_time_zone(..., None)
)?
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.
Yep you're right, that's also where I got the logic from.
However, replace_time_zone
is in polar-ops
, which depends on polars-core
.
Then datetime.rs
is in polars-core
, so we'd have a circular dependency when we'd use something from polars-ops
in polars-core
.
Context
This fixes #22864
DateTime stores the timestamp in unix time, which leads to inconsistencies when timezones are used.
This for example leads to an incorrect cast to date, which previously acted as if no timezone was present at all.
I used
chrono
to implement the parsing - let me know if there's a more idiomatic approach to this that doesn't add to much boilerplate to polars.