matko commented on code in PR #1000:
URL:
https://github.com/apache/datafusion-python/pull/1000#discussion_r1932009641
##########
src/pyarrow_util.rs:
##########
Review Comment:
it would be good to have the conversion functions in this file also
available directly on a `ScalarValue`, which is how it was provided by the
pyarrow feature. This to prevent having to clone `ScalarValue`s, just to wrap
them in a `PyScalarValue` to get access to these functions.
I understand we can't implement traits like `ToPyArrow` directly on
`ScalarValue` as it is not in this crate, but we can leave it as an ordinary
utility function.
```rust
pub fn scalar_to_pyarrow(scalar: &ScalarValue) -> PyResult<PyObject> {
...
}
```
Then call it from `ToPyArrow` as a common code path.
##########
src/expr.rs:
##########
@@ -356,7 +355,7 @@ impl PyExpr {
/// Extracts the Expr value into a PyObject that can be shared with Python
pub fn python_value(&self, py: Python) -> PyResult<PyObject> {
match &self.expr {
- Expr::Literal(scalar_value) => Ok(scalar_value.to_pyarrow(py)?),
+ Expr::Literal(scalar_value) =>
Ok(PyScalarValue(scalar_value.clone()).to_pyarrow(py)?),
Review Comment:
This is now a double clone, as `to_pyarrow()` is also cloning eventually. I
see that this previously called into the pyarrow feature-enabled code, and this
now got replaced by a version that works on `PyScalarValue` instead. However,
we could still have a version of this code that works directly on
`ScalarValue`. See notes below at pyarrow_util.rs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
