ScalarUDF with zero arguments should be provided with one null array as parameter #9031
Conversation
github-actions
bot
added
physical-expr
|
Fix clippy at #9034 |
viirya
force-pushed
the
fix_udf_zero_args
branch
from
5506837
to
aeb91b2
Compare
| @@ -578,8 +578,9 @@ fn roundtrip_builtin_scalar_function() -> Result<()> { | |||
| "acos", | |||
| fun_expr, | |||
| vec![col("a", &schema)?], | |||
| DataType::Int64, | |||
| DataType::Float64, | |||
There was a problem hiding this comment.
The existing test is not correct at all. acos built-in scalar function's return type should be Float64.
Previously the roundtrip test passes because from_proto simply takes serde return type and uses it as parameter to ScalarFunctionExpr.
But in this PR, from_proto calls create_physical_expr which gets return type directly from BuiltinScalarFunction. So with the PR, this test issue is found.
| async fn test_user_defined_functions_zero_argument() -> Result<()> { | ||
| let ctx = SessionContext::new(); | ||
|
|
||
| let schema = Arc::new(Schema::new(vec![ |
There was a problem hiding this comment.
it doesn't hurt but I wonder if the example table needs 4 columns 🤔
There was a problem hiding this comment.
I just copied the reported test case. I think we can reduce the columns.
|
|
||
| assert_eq!(random_udf.len(), native_random.len()); | ||
|
|
||
| let mut previous = 1.0; |
There was a problem hiding this comment.
Can could the random implementation ever actually make 1.0 (the range is 0..1.0). Maybe we could start at -1.0 or something just to be sure this won't ever flake
There was a problem hiding this comment.
I think the range 0..1.0 is exclusive on the end point?
There was a problem hiding this comment.
I think the range 0..1.0 is exclusive on the end point?
If so that this is fine!
There was a problem hiding this comment.
Changed to -1.0 to make it more clear.
| @@ -58,6 +58,8 @@ pub struct ScalarFunctionExpr { | |||
| // and it specifies the effect of an increase or decrease in | |||
| // the corresponding `arg` to the function value. | |||
| monotonicity: Option<FuncMonotonicity>, | |||
| // Signature of the function | |||
| signature: Signature, | |||
There was a problem hiding this comment.
It seems like only one field is ever read. I wonder if it would be better to copy just this field rather than the entire signature (which is both larger with several allocations, but also might be misleading that this signature information was used somehow more in execution plans.
I worry that the signature information might start being referred to in physical planning
So perhaps something like
| signature: Signature, | |
| // Does this function need to be invoked with zero arguments ? | |
| supports_zero_argument: bool, |
self.signature.type_signature.supports_zero_argument| @@ -149,6 +153,11 @@ impl PhysicalExpr for ScalarFunctionExpr { | |||
| { | |||
| vec![ColumnarValue::create_null_array(batch.num_rows())] | |||
| } | |||
| // If the function supports zero argument, we pass in a null array indicating the batch size. | |||
There was a problem hiding this comment.
I never fully understood why this didn't just check self.args.is_empty() 🤔
There was a problem hiding this comment.
Good idea. Changed to self.args.is_empty().
Which issue does this PR close?
Closes #9032.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?