Minor: add arguments length check in array_expressions
#8622
Conversation
|
I thought we should handle this in array signature 🤔 Other than lengths, whether it is list or not, null or empty array handling etc. Those checking may be done in array signature step. I think doing the same thing in different place can be avoided. |
| @@ -1082,6 +1106,10 @@ fn concat_internal(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
|
|
|||
| /// Array_concat/Array_cat SQL function | |||
| pub fn array_concat(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
| if args.is_empty() { | |||
| return exec_err!("array_concat expects at least one arguments"); | |||
There was a problem hiding this comment.
| return exec_err!("array_concat expects at least one arguments"); | |
| return exec_err!("array_concat expects at least one argument"); |
| @@ -883,6 +899,10 @@ pub fn array_append(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
|
|
|||
| /// Array_sort SQL function | |||
| pub fn array_sort(args: &[ArrayRef]) -> Result<ArrayRef> { | |||
| if args.is_empty() || args.len() > 3 { | |||
There was a problem hiding this comment.
I think we need to make it more usable in future for all built in functions. To provide the exact available signatures, to let user quickly find what is his mistake
There was a problem hiding this comment.
I think we need to make it more usable in future for all built in functions. To provide the exact available signatures, to let user quickly find what is his mistake
@comphead
But we will also handle this while checking signature of array. Why do we need to check it here again? Is there any usage that skip the signature checking but jump to this call directly? If yes, we need to find a way to reuse the checking because the coercion is not supported here currently, length checking may not be enough.
There was a problem hiding this comment.
Checking input arguments validity in public methods is good practice imho.
But the comment above was mostly to thinking on having a generic approach to return a friendly message to the user about what is wrong and what is the next step. Now we usually saying the input arguments has to be 1, 2, 3 arguments but it is mostly meaningless, we can probably improve it with more detailed message
There was a problem hiding this comment.
Sorry for the late reply. I agree with the message that more specific information would be better.
Which issue does this PR close?
As suggestions by @comphead in #8592 (comment), I found many functions in
array_expression.rslack the argument check.Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?