-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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(python): Validate time_zone
in pl.Datetime
#16165
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16165 +/- ##
==========================================
- Coverage 80.99% 80.98% -0.01%
==========================================
Files 1387 1387
Lines 178832 178842 +10
Branches 2877 2880 +3
==========================================
+ Hits 144839 144841 +2
- Misses 33500 33506 +6
- Partials 493 495 +2 ☔ 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.
This looks pretty good! I left some comments. Would be nice if @MarcoGorelli could also take a look at this.
else: | ||
msg = "install polars[timezone] to handle datetimes with time zone information" | ||
raise ImportError(msg) |
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 think we should skip validation in this case, rather than raise an error. I believe working with time zones only requires the zoneinfo package in some specific cases.
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.
Thank you for the advice, I have removed the error.
from polars.dependencies import _ZONEINFO_AVAILABLE, zoneinfo | ||
|
||
if _ZONEINFO_AVAILABLE: | ||
if time_zone in zoneinfo.available_timezones(): |
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.
Does this match with the list of timezones we use on the Rust side? What are the differences if there are any?
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.
This is the part that concerns me a little, I'm pretty sure I encountered some differences
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 having worked on this
To be honest I'm a bit hesitant about it - it's already impossible (counterexamples welcome, I might be missing something) to create a column with an invalid time zone
Not sure there's an advantage to adding extra code to make the validation happen earlier, especially if it doesn't quite match how the validation is done at the Rust level
Actually, when I was writing the code, I was concerned about whether it was okay to add extra validation in I'm not sure if you noticed this issue comment. What are your thoughts on it? If we decide to leave the check for later, should we add some notes in the user guide or the method's documentation to clarify that this is what we intend? |
closes #16038
Before
After