Skip to content

Simpify PyExpr::python_value by using ScalarValue::into_py #729

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
Michael-J-Ward opened this issue Jun 13, 2024 · 0 comments · Fixed by #731
Closed

Simpify PyExpr::python_value by using ScalarValue::into_py #729

Michael-J-Ward opened this issue Jun 13, 2024 · 0 comments · Fixed by #731
Labels
enhancement New feature or request

Comments

@Michael-J-Ward
Copy link
Contributor

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
Currently, we de-structure every ScalarValue variant

pub fn python_value(&self, py: Python) -> PyResult<PyObject> {
match &self.expr {
Expr::Literal(scalar_value) => match scalar_value {
ScalarValue::Null => Err(py_datafusion_err(
datafusion_common::DataFusionError::NotImplemented(
"ScalarValue::Null".to_string(),
),
)),
ScalarValue::Boolean(v) => Ok(v.into_py(py)),
ScalarValue::Float32(v) => Ok(v.into_py(py)),
ScalarValue::Float64(v) => Ok(v.into_py(py)),
ScalarValue::Decimal128(v, _, _) => Ok(v.into_py(py)),

But ScalarValue already implements IntoPy:
https://github.com/apache/datafusion/blob/b7d2aea1dd4bb4a3abe3163dae936d7bfa5b32c9/datafusion/common/src/pyarrow.rs#L72-L76

If we want to avoid that unwrap, we could use the ToPyArrow trait:
https://github.com/apache/datafusion/blob/b7d2aea1dd4bb4a3abe3163dae936d7bfa5b32c9/datafusion/common/src/pyarrow.rs#L55-L64

Additional context
Are there other constraints the ScalarValue variants that datafusion-python can support other than what datafusion can convert?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant