chore(python): Continue converting to PyO3 0.21 Bound<> APIs #16081
Conversation
github-actions
bot
added
internal
itamarst
requested review from
ritchie46,
stinodego,
c-peters,
alexander-beedie,
MarcoGorelli and
reswqa
as code owners
stinodego
changed the title
|
Will use correct tag for future PRs, thanks. Also note that I can't merge this, so someone else will have to :) |
| @@ -46,7 +48,7 @@ impl PyExpr { | |||
| let name_mapper = Arc::new(move |name: &str| { | |||
| Python::with_gil(|py| { | |||
| let out = name_mapper.call1(py, (name,)).unwrap(); | |||
| let out: SmartString = out.extract::<&str>(py).unwrap().into(); | |||
| let out: SmartString = out.extract::<Cow<str>>(py).unwrap().into(); | |||
There was a problem hiding this comment.
Why do we use Cow in favor of pybackedstr here?
There was a problem hiding this comment.
Less typing to convert to SmartString. Could do it the other way too, though.
There was a problem hiding this comment.
Ok, looking at the FromPyObject implementations in Pyo3, I now see what the "fastest" way is to get a rust &str.
And that is indeed going via Cow. As they write obj.downcast::<PyString>()?.to_cow().map(Cow::into_owned) for String, where to_cow says:
"""
Converts the PyString into a Rust string, avoiding copying when possible.
"""
So I do think this is the fastest way to get a borrowing string out of python. I think we should favor this than over pybytes in other places.
There was a problem hiding this comment.
- Is that specific to this PR too? If not, can it be merged?
- I am not sure that
Cow<str>is always the best option. In particular, it has a lifetime that is tied to the original object.PyBackedStrin contrast is an owned object, so it's a lot easier to work with some contexts. And it has some overhead but I don't think its copying overhead, it's more "having aPy<>" overhead, so like... a Python incref basically?
There was a problem hiding this comment.
Yeah, I mean in the borrowing &str case. Not related to this PR indeed. Thanks!
…#16081) Co-authored-by: Itamar Turner-Trauring <[email protected]>
Fixes #16078