From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp53.i.mail.ru (smtp53.i.mail.ru [94.100.177.113]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 01735445320 for ; Mon, 13 Jul 2020 15:21:01 +0300 (MSK) Date: Mon, 13 Jul 2020 12:21:01 +0000 From: Nikita Pettik Message-ID: <20200713122100.GC15396@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v4 3/5] sql: check number of arguments in sql_func_by_signature() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: imeevma@tarantool.org Cc: tarantool-patches@dev.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 > ]], { > -- > - 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" > -- > }) > > @@ -488,7 +488,7 @@ test:do_catchsql_test( > SELECT round() FROM t1 ORDER BY a > ]], { > -- > - 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" > -- > }) > > @@ -2540,7 +2540,7 @@ test:do_catchsql_test( > SELECT coalesce() > ]], { > -- > - 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??).