[Tarantool-patches] [PATCH v4 3/5] sql: check number of arguments in sql_func_by_signature()
Nikita Pettik
korablev at tarantool.org
Mon Jul 13 15:21:01 MSK 2020
On 13 Jul 08:33, imeevma at 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??).
More information about the Tarantool-patches
mailing list