Handle one-element array return value in ScalarFunctionExpr #12965
Conversation
github-actions
bot
added
physical-expr
| // If the function is not volatile and all arguments are scalars, | ||
| // we can assume that returning a one-element array is equivalent to returning a scalar. | ||
| let preserve_scalar = array.len() == 1 | ||
| && self.fun.signature().volatility != Volatility::Volatile |
There was a problem hiding this comment.
Why can we not do this for functions that are Volatile?
There was a problem hiding this comment.
A volatile function is supposed to return a different result on every invocation so I don't think it's safe to do that. The example we were discussing in the earlier PR is a hypothetical rand(upper_bound: Int64) -> Int64 function.
There was a problem hiding this comment.
I read the discussion and I see where you coming from. I guess a bit confusing since that function is currently not implementable like you said in that thread.
There was a problem hiding this comment.
I guess a bit confusing since that function is currently not implementable like you said in that thread.
I think so. It's currently not implementable.
I think under the current UDF invoke model, we should preserve scalar even if it is volatile. For example, we have a UDF fn random_string(len: int) -> String, which returns a random string of length len. This function is volatile, but it should return a scalar if the len parameter is a scalar.
There was a problem hiding this comment.
Sure, I can make that change - was just leaning more on the safe side.
| && self.fun.signature().volatility != Volatility::Volatile | ||
| && inputs | ||
| .iter() | ||
| .all(|arg| matches!(arg, ColumnarValue::Scalar(_))); |
There was a problem hiding this comment.
I think we should consider the case where the inputs are empty. For UDFs without args, either return a scalar directly or return an array with num_rows. Trying to convert the output array back to scalar for them seems unnecessary.
This was done in apache#12922 only for math functions. We now generalize this fallback to all scalar UDFs.
|
Thanks @joroKr21 @eejbyfeldt and @jonahgao |
…2965) This was done in apache#12922 only for math functions. We now generalize this fallback to all scalar UDFs.
…2965) (#276) This was done in apache#12922 only for math functions. We now generalize this fallback to all scalar UDFs.
…2965) (#276) This was done in apache#12922 only for math functions. We now generalize this fallback to all scalar UDFs.
…2965) (#276) This was done in apache#12922 only for math functions. We now generalize this fallback to all scalar UDFs.
…2965) (#276) This was done in apache#12922 only for math functions. We now generalize this fallback to all scalar UDFs.
…2965) (#276) This was done in apache#12922 only for math functions. We now generalize this fallback to all scalar UDFs.
4 participants
This was done in #12922 only for math functions.
We now generalize this fallback to all scalar UDFs.
Which issue does this PR close?
Closes #12959
Rationale for this change
See #12922
What changes are included in this PR?
ScalarFunctionExprAre these changes tested?
Relying on existing tests.
Are there any user-facing changes?
ColumnarValue::from_args_and_resultis removed again, it's not released yet.