alamb commented on code in PR #10193:
URL: https://github.com/apache/datafusion/pull/10193#discussion_r1575980985
##########
datafusion/functions/src/math/random.rs:
##########
@@ -69,12 +69,13 @@ impl ScalarUDFImpl for RandomFunc {
}
}
-/// Random SQL function
fn random(args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect random function to take no param"),
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return exec_err!("Expect random function to take no param");
Review Comment:
This might be more consistent with the other changes in this PR:
```suggestion
return exec_err!("Expect {} function to take no parameters",
self.name());
```
##########
datafusion/functions/src/math/pi.rs:
##########
@@ -63,9 +63,10 @@ impl ScalarUDFImpl for PiFunc {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- if !matches!(&args[0], ColumnarValue::Array(_)) {
- return exec_err!("Expect pi function to take no param");
- }
+ if !args.is_empty() {
+ return exec_err!("Expect {} function to take no param",
self.name());
Review Comment:
```suggestion
return exec_err!("Expect {} function to take no parameters",
self.name());
```
##########
datafusion/core/tests/user_defined/user_defined_scalar_functions.rs:
##########
@@ -434,15 +434,10 @@ impl ScalarUDFImpl for RandomUDF {
}
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- // This udf is always invoked with zero argument so its argument
- // is a null array indicating the batch size.
- ColumnarValue::Array(array) if array.data_type().is_null() =>
array.len(),
- _ => {
- return Err(datafusion::error::DataFusionError::Internal(
- "Invalid argument type".to_string(),
- ))
- }
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return internal_err!("Invalid argument type");
Review Comment:
It might be good to have the test match the rest of the functions for
consistency
```suggestion
return exec_err!("Expect function to take no parameters");
```
##########
datafusion/functions/src/string/uuid.rs:
##########
@@ -61,9 +61,10 @@ impl ScalarUDFImpl for UuidFunc {
/// Prints random (v4) uuid values per row
/// uuid() = 'a0eebc99-9c0b-4ef8-bb6d-6bb9bd380a11'
fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
- let len: usize = match &args[0] {
- ColumnarValue::Array(array) => array.len(),
- _ => return exec_err!("Expect uuid function to take no param"),
+ let len = if args.is_empty() {
+ 1
+ } else {
+ return exec_err!("Expect {} function to take no param",
self.name());
Review Comment:
```suggestion
return exec_err!("Expect {} function to take no parameters",
self.name());
```
##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -142,21 +142,11 @@ impl PhysicalExpr for ScalarFunctionExpr {
}
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
- // evaluate the arguments, if there are no arguments we'll instead
pass in a null array
- // indicating the batch size (as a convention)
- let inputs = match self.args.is_empty() {
- // If the function supports zero argument, we pass in a null array
indicating the batch size.
- // This is for user-defined functions.
- // MakeArray support zero argument but has the different behavior
from the array with one null.
Review Comment:
So is the thinking `support_zero_argument` is no longer needed? Should that
field be removed entirely now?
If all the existing tests pass this certainly seems like a reasonable
special case to remove (maybe we could update the documentation on
`ScalarUDFImpl` to explain how to handle funcitons that take zero arguments --
namely to return a single row array (or ColumnarValue)
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
