[ES|QL][SHOW FUNCTION] Change variadic annotation to minArgs

[dej611]

Change the variadic column (boolean) into the numeric minArgs column.

Fix part of #100621 and #106200 .

Why this change?

ES|QL has few variadic functions, but some of them requires a minimum number of args and this information is not provided elsewhere other than the code itself.
The minArgs annotation is a superset of the variadic one (equivalent to minArgs > 0) and it provides additional information about the minimum number of arguments a variadic function accepts.

With this change any client (i.e. Kibana) can leverage this information to provide better services for ES|QL functions (i.e. validation, autocomplete, etc...).

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[elasticsearchmachine]

Hi @dej611, I've created a changelog YAML for you.

[astefan]

The type and what it represents is confusing. minArgs indicates a number while the type is String?
And looking at show.csv-spec I see mostly null values and here and there 1 for minArgs. What does null represent?

[dej611]

Here's the minArgs values defined:

• concat: 2
• case: 3 2
• coalesce: 1
• greatest: 1
• least: 1
• cidr_match: 2

I had to adopt null here to make tests pass. Empty values were my initial implementation but tests were complaining. With null it passes.

[dej611]

The type and what it represents is confusing. minArgs indicates a number while the type is String?

I thought about adopting int at first, but given its small footprint using String avoided changes for most part of the code around. I can refactor to it, hoping all the testing logic won't be too affected.

[astefan]

Also, why minArgs is a replacement for variadic? I think these two have two different purposes and they are not equivalent.

[dej611]

Also, why minArgs is a replacement for variadic? I think these two have two different purposes and they are not equivalent.

I think the misunderstanding is the naming I've chosen here. Perhaps variadicMinArgs can be a better column label there.
The main point is to indicate how many arguments are required for a variadic function.

[astefan]

We discussed this offline and the minArgs value can be taken from the length of the optionalArgs column when the isVariadic value is true. There is no need for the change in this PR, thus closing it. Thank you @dej611

[craigtaverner]

We discussed this offline and the minArgs value can be taken from the length of the optionalArgs column when the isVariadic value is true. There is no need for the change in this PR, thus closing it. Thank you @dej611

This does not seem to be correct. For coalesce, there are two args, and the optionalArgs column has length 2 (and value [false, false]), with the second argument being variadic, and having 0 or more values. This leads to the function supporting 1 or more arguments, not 2 or more. So minArgs would be 1, not 2, and is not the length of the optionalArgs column.

Perhaps minArgs is:

• When variadic = false: minArgs = length(optionalArgs)
• When variadic = true: minArgs = length(optionalArgs) - 1

[astefan]

@craigtaverner fair point, but I think the problem is with the coalesce info, not with the approach in general. The fact that coalesce shows two required parameters is wrong. Our documentation says

Syntax
COALESCE(expression1 [, ..., expressionN])

and yes, coalesce accepts one parameter only. This means that [false, false] is wrong as well. It should either be [false] or [false, true]. The same wrong output is for greatest and least, at least.

concat is again inconsistent:

     name      |   variadic    | optionalArgs  |        argNames         
concat         |true           |[false, false] |[first, rest]            

optionalArgs is correct: it has two required parameters, but what about the third (variadic) parameter (like we do for coalesce, case and others)?

In conclusion:

• you are correct that something is wrong, but it shouldn't be length(optionalArgs) - 1 since the inconsistencies expand beyond coalesce and are not consistent with length(optionalArgs) - 1 assumption.
• we need a complete review of the show functions approach and strive for a consistent approach so that the idea of "minimum number of required arguments" has a solid foundation.

I've created #106346. After we review all functions, the rule for "minimum number of required arguments" will be length(optionalArgs) or number of "false" values in optionalArgs when variadic=true.

[dej611]

Perhaps minArgs is:

• When variadic = false: minArgs = length(optionalArgs)
• When variadic = true: minArgs = length(optionalArgs) - 1

When variadic is false there's no problem with the counting, as the minimal number of accepted args it is just |allArgs| - |optionalArgs|.
As for variadic = true I've ran this query: show functions | where variadic | keep name, optionalArgs, variadic:

name	optionalArgs	variadic
case	[false, false]	true
cidr_match	[false, false]	true
coalesce	[false, false]	true
concat	[false, false]	true
greatest	[false, false]	true
least	[false, false]	true

And here's a list of the expected minArgs vs current value when optionalArgs:

name	expected minArgs	from optionaArgs	Δ
case	2	2	0 ✅
cidr_match	2	2	0 ✅
coalesce	1	2	-1 ❌
concat	2	2	0 ✅
greatest	1	2	-1 ❌
least	1	2	-1 ❌

I see here 3 possible ways to solve this challenge:

• change the optionalArgs value for those 3 functions that do not met the criteria (and add a test suite for regressions)
• re-open this PR and manually annotate the minArgs for variadic functions
• make all three functions who currently accepts 1 minArg, to align with the rest.