fix: Allow for date/datetime subclasses (e.g. pd.Timestamp, FreezeGun) in pl.lit #18497
Conversation
MarcoGorelli
changed the title
github-actions
bot
added
title needs formatting
fix
MarcoGorelli
force-pushed
the
datetime-subclasses
branch
from
f260a44
to
0eab763
Compare
MarcoGorelli
force-pushed
the
datetime-subclasses
branch
from
0eab763
to
633b000
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18497 +/- ##
==========================================
- Coverage 79.82% 79.82% -0.01%
==========================================
Files 1502 1502
Lines 201933 201942 +9
Branches 2868 2868
==========================================
- Hits 161201 161193 -8
- Misses 40186 40203 +17
Partials 546 546 ☔ View full report in Codecov by Sentry. |
MarcoGorelli
requested review from
ritchie46,
c-peters,
alexander-beedie,
reswqa and
orlp
as code owners
| Python::with_gil(|py| { | ||
| // One final attempt before erroring. Do we have a date/datetime subclass? | ||
| // E.g. pd.Timestamp, or Freezegun. | ||
| let datetime_module = PyModule::import_bound(py, "datetime")?; | ||
| let datetime_class = datetime_module.getattr("datetime")?; | ||
| let date_class = datetime_module.getattr("date")?; | ||
| if value.is_instance(&datetime_class)? || value.is_instance(&date_class)? { | ||
| let av = py_object_to_any_value(value, true)?; | ||
| Ok(Expr::Literal(LiteralValue::try_from(av).unwrap()).into()) |
There was a problem hiding this comment.
Pyo3 does have PyDateTime, but it's limited to:
Available on non-Py_LIMITED_API only
There was a problem hiding this comment.
This should come before the allow_object check.
I am thinking aloud here. Why don't we go via AnyValue directly for all branches? This seems like code duplication.
There was a problem hiding this comment.
yup, thanks for catching that, and sorry for not having spotted it myself
fixed, and I've added a test which would've on my previous commit to make sure this doesn't regress
There was a problem hiding this comment.
Why don't we go via AnyValue directly for all branches?
Tried then, but then a float would become
[crates/polars-python/src/functions/lazy.rs:429:9] &newret = Ok(
PyExpr {
inner: 2.0,
},
)
instead of
[crates/polars-python/src/functions/lazy.rs:432:9] &oldret = Ok(
PyExpr {
inner: dyn float: 2.0,
},
)
, and I haven't (yet) dived deeper here
There was a problem hiding this comment.
Ah, yeap.. That's a good reason.
|
Ahh sorry Marco dropped the ball on this one. I was looking for a way to avoid acquiring the gil again since |
closes #18470
closes #18447