Re: [Tarantool-patches] [PATCH v4 3/5] sql: check number of arguments in sql_func_by_signature()

[Nikita Pettik <korablev@tarantool.org>]

On 13 Jul 08:33, imeevma@tarantool.org wrote:
> After this patch, the number of function arguments will always be
> checked in the sql_func_by_signature() function. This was not the
> case for some of the built-in functions.

At least, I'd split patch into two parts: first introduces new
members and changes data structures, and the second one - adds
new check (if it is necessary at all; see comments below).
 
> Part of #4159
> ---
> @@ -2195,11 +2164,28 @@ sql_func_by_signature(const char *name, int argc)
>  				     name));
>  		return NULL;
>  	}

It looks strange since other 'by' getters (space, index, collation)
dont set diag errors.

> -	int param_count = base->def->param_count;
> -	if (param_count != -1 && param_count != argc) {
> -		const char *err = tt_sprintf("%d", param_count);
> -		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
> -			 base->def->name, err, argc);
> +	if (base->def->language != FUNC_LANGUAGE_SQL_BUILTIN) {
> +		int param_count = base->def->param_count;
> +		if (param_count != -1 && param_count != argc) {
> +			const char *err = tt_sprintf("%d", param_count);
> +			diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT,
> +				 base->def->name, err, argc);
> +			return NULL;
> +		}
> +		return base;
> +	}
> +	struct func_sql_builtin *func = (struct func_sql_builtin *)base;
> +	uint32_t arg_c = (uint32_t)argc;
> +	if (func->args.min_count > arg_c || func->args.max_count < arg_c) {
> +		const char *err;
> +		uint32_t min = func->args.min_count;
> +		uint32_t max = func->args.max_count;
> +		if (min != max)
> +			err = tt_sprintf("from %d to %d", min, max);
> +		else
> +			err = tt_sprintf("%d", min);
> +		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, base->def->name,
> +			 err, argc);
>  		return NULL;
>  	}
>  	return base;
> @@ -2242,12 +2228,16 @@ static struct {
>  	/** Members below are related to struct func_def. */
>  	bool is_deterministic;
>  	int param_count;
> +	uint32_t min_count;
> +	uint32_t max_count;

Why not erase param_count?
min/max_count of what? Also add comments to these members explaining
why do we need them at all.
Why can't we use -1 param_count for variadic args and provide check
in func's implementation?

>  	enum field_type returns;
>  	enum func_aggregate aggregate;
>  	bool export_to_sql;
>  } sql_builtins[] = {
>  	{.name = "ABS",
>  	 .param_count = 1,
> +	 .min_count = 1,
> +	 .max_count = 1,
>  	 .returns = FIELD_TYPE_NUMBER,
>  	 .aggregate = FUNC_AGGREGATE_NONE,
>  	 .is_deterministic = true,
> @@ -2318,6 +2320,8 @@ static struct {
>  	}, {
>  	 .name = "COALESCE",
>  	 .param_count = -1,
> +	 .min_count = 2,
> +	 .max_count = SQL_MAX_FUNCTION_ARG,

Why do you need uint32 if max value fits into uint8?

> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 58a65acc1..6af9d7473 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4397,11 +4397,24 @@ Expr *sqlExprForVectorField(Parse *, Expr *, int);
>   */
>  extern int sqlSubProgramsRemaining;
>  
> +/**
> + * A structure that contains additional information about
> + * arguments to built-in SQL functions.
> + */
> +struct sql_builtin_func_args {
> +	/** Min number of arguments. */
> +	uint32_t min_count;
> +	/** Max number of arguments. */
> +	uint32_t max_count;
> +};
> +
>  struct func_sql_builtin {
>  	/** Function object base class. */
>  	struct func base;
>  	/** A bitmask of SQL flags. */
>  	uint16_t flags;
> +	/** Information about arguments to built-in functions. */
> +	struct sql_builtin_func_args args;

Why not simply inline two members (min/max)?

>  	/**
>  	 * A VDBE-memory-compatible call method.
>  	 * SQL built-ins don't use func base class "call"
> diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
> index 3c088920f..1d3ef9e2a 100755
> --- a/test/sql-tap/func.test.lua
> +++ b/test/sql-tap/func.test.lua
> @@ -428,7 +428,7 @@ test:do_catchsql_test(
>          SELECT round(a,b,c) FROM t1
>      ]], {
>          -- <func-4.5>
> -        1, "Wrong number of arguments is passed to ROUND(): expected 1 or 2, got 3"
> +        1, "Wrong number of arguments is passed to ROUND(): expected from 1 to 2, got 3"
>          -- </func-4.5>
>      })
>  
> @@ -488,7 +488,7 @@ test:do_catchsql_test(
>          SELECT round() FROM t1 ORDER BY a
>      ]], {
>          -- <func-4.11>
> -        1, "Wrong number of arguments is passed to ROUND(): expected 1 or 2, got 0"
> +        1, "Wrong number of arguments is passed to ROUND(): expected from 1 to 2, got 0"
>          -- </func-4.11>
>      })
>  
> @@ -2540,7 +2540,7 @@ test:do_catchsql_test(
>          SELECT coalesce()
>      ]], {
>          -- <func-27.1>
> -        1, "Wrong number of arguments is passed to COALESCE(): expected at least two, got 0"
> +        1, "Wrong number of arguments is passed to COALESCE(): expected from 2 to 127, got 0"

Current error message looks better than new one (127 arguments??).