alamb commented on code in PR #13637:
URL: https://github.com/apache/datafusion/pull/13637#discussion_r1868550990
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -134,20 +134,20 @@ impl PhysicalExpr for ScalarFunctionExpr {
}
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
Review Comment:
I think we could potentially increase the number of places arrays could be
reused by changing this to take `batch` (owned) and threading that more fully
through the physical exprs.
Until there is some compelling usecase to do so, however, I don't think we
should go mess with the APIs again
##########
datafusion-examples/examples/advanced_udf.rs:
##########
@@ -191,6 +199,48 @@ impl ScalarUDFImpl for PowUdf {
}
}
+/// Evaluate `base ^ exp` *without* allocating a new array, if possible
+fn pow_in_place(base: f64, exp_array: ArrayRef) -> Result<ArrayRef> {
Review Comment:
Here is the example of how to write a function that reuses the input
argument's allocation
##########
datafusion/expr/src/udf.rs:
##########
@@ -326,13 +326,15 @@ where
}
}
+/// Arguments passed to [`ScalarUDFImpl::invoke_with_args`] when invoking a
+/// scalar function.
pub struct ScalarFunctionArgs<'a> {
- // The evaluated arguments to the function
- pub args: &'a [ColumnarValue],
- // The number of rows in record batch being evaluated
+ /// The evaluated arguments to the function
+ pub args: Vec<ColumnarValue>,
Review Comment:
this is an API change, but since this struct has not been released yet, it
isn't a user facing change.
##########
datafusion-examples/examples/advanced_udf.rs:
##########
@@ -83,23 +85,27 @@ impl ScalarUDFImpl for PowUdf {
Ok(DataType::Float64)
}
- /// This is the function that actually calculates the results.
+ /// This function actually calculates the results of the scalar argument
+ ///
+ /// This is the same way that functions provided with DataFusion are
invoked,
+ /// which permits important special cases:
///
- /// This is the same way that functions built into DataFusion are invoked,
- /// which permits important special cases when one or both of the arguments
- /// are single values (constants). For example `pow(a, 2)`
+ ///1. When one or both of the arguments are single values (constants).
Review Comment:
I updated the advanced_udf example to show how to update the arrays in place
##########
datafusion/functions/src/datetime/to_local_time.rs:
##########
@@ -562,7 +562,7 @@ mod tests {
fn test_to_local_time_helper(input: ScalarValue, expected: ScalarValue) {
let res = ToLocalTimeFunc::new()
.invoke_with_args(ScalarFunctionArgs {
- args: &[ColumnarValue::Scalar(input)],
+ args: vec![ColumnarValue::Scalar(input)],
Review Comment:
Most of the rest of this PR is changes for the new signature
--
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]
