-
-
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: Improve literals for temporal subclasses #18998
Conversation
} else if ob.is_instance_of::<PyDict>() { | ||
get_struct | ||
} else if ob.hasattr(intern!(py, "_s")).unwrap() { | ||
get_list_from_series |
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 was unused as far as I can tell. Removing it didn't fail any tests. The path to a literal list is elsewhere.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18998 +/- ##
=======================================
Coverage 79.88% 79.88%
=======================================
Files 1524 1524
Lines 207634 207643 +9
Branches 2904 2904
=======================================
+ Hits 165873 165882 +9
Misses 41214 41214
Partials 547 547 ☔ View full report in Codecov by Sentry. |
I'm unable to replicate the failing test, even after installing same Python (3.12.16). @MarcoGorelli any ideas? Edit: rebased and it went away. |
b52aaac
to
a61f379
Compare
Thanks @mcrumiller. I think this is a good improvement as we now convert in one location. |
@mcrumiller can you take a look. This test failed in the ci: https://github.com/pola-rs/polars/actions/runs/11093722576/job/30820360138?pr=18930 |
Yikes, sorry. Yeah let me look. |
Must be something about installed timezone modules, I can't reproduce. I'll work on the setup to figure out what's going on. |
For some reason |
New theory: same class name defined in same function in multiple threads are colliding, even with different inheritence definitions. |
@ritchie46 I've added PR #19012 which I think resolves the issue. |
Fixes #18906.
This update pushes
allow_object
into theAnyValue
conversion, and by doing so avoids the double-gil lock for subclassed items (@ritchie46 I think it's what you were thinking in this comment). I think it should help tidy up some of the subclass issues with temporals. I also added checks ontime
andtimedelta
temporal classes as well, as is the case in the linked issue wherepd.Timedelta
subclassesdatetime.timedelta
.