From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9F4DF24957 for ; Thu, 22 Aug 2019 10:37:42 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jFMNUVZ7nA-X for ; Thu, 22 Aug 2019 10:37:42 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id D17CF21FB0 for ; Thu, 22 Aug 2019 10:37:41 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH v4 3/4] sql: get rid of FuncDef function hash From: "n.pettik" In-Reply-To: <154ee8ea11ae260254b71d32e1299f1272438af1.1566400979.git.kshcherbatov@tarantool.org> Date: Thu, 22 Aug 2019 17:37:39 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <17C6E71F-24DF-476D-B5FA-430AF34DC84D@tarantool.org> References: <154ee8ea11ae260254b71d32e1299f1272438af1.1566400979.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov > @@ -1316,11 +1227,10 @@ struct FuncDestructor { > #define SQL_FUNC_COUNT 0x0100 > #define SQL_FUNC_COALESCE 0x0200 /* Built-in coalesce() or = ifnull() */ > #define SQL_FUNC_UNLIKELY 0x0400 /* Built-in unlikely() function = */ > -#define SQL_FUNC_CONSTANT 0x0800 /* Constant inputs give a = constant output */ > /** Built-in min() or least() function. */ > #define SQL_FUNC_MIN 0x1000 > /** Built-in max() or greatest() function. */ > -#define SQL_FUNC_MAX 0x2000 > +#define SQL_FUNC_MAX 0x2000 Nit: extra diff. > @@ -4489,9 +4331,55 @@ Expr *sqlExprForVectorField(Parse *, Expr *, = int); > */ > extern int sqlSubProgramsRemaining; >=20 > -/** Register built-in functions to work with ANALYZE data. */ > -void > -sql_register_analyze_builtins(void); > +struct func_sql_builtin { > + /** Function object base class. */ > + struct func base; > + /** A bitmask of SQL flags. */ > + uint16_t flags; > + /** > + * A VDBE-memory-compatible call method. > + * SQL built-ins don't use func base class "call" > + * method to provide a best performance for SQL requests. > + * Access checks are redundant, because all SQL built-ins > + * are predefined and are executed on SQL privilege level. Which doesn=E2=80=99t exist yet=E2=80=A6 I asked you to document or fix = it. Comment in source code is OK, but it should be present in documentation as well. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 1f9d91705..9055a0770 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -4002,9 +3999,16 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) > * IFNULL() functions. This avoids unnecessary = evaluation of > * arguments past the first non-NULL argument. > */ > - if (pDef->funcFlags & SQL_FUNC_COALESCE) { > + if (sql_func_flag_is_set(func, = SQL_FUNC_COALESCE)) { > int endCoalesce =3D sqlVdbeMakeLabel(v); > - assert(nFarg >=3D 2); > + if (nFarg < 2) { > + diag_set(ClientError, > + = ER_FUNC_WRONG_ARG_COUNT, > + func->def->name, > + ">=3D 2", nFarg); -> =E2=80=9Cmore than one=E2=80=9D/=E2=80=9Cat least two=E2=80=9D What is more, you can move introduction of ER_FUNC_WRONG_=E2=80=A6 to a separate auxiliary patch. > @@ -4026,8 +4030,15 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, = int target) > /* The UNLIKELY() function is a no-op. The = result is the value > * of the first argument. > */ > - if (pDef->funcFlags & SQL_FUNC_UNLIKELY) { > - assert(nFarg >=3D 1); > + if (sql_func_flag_is_set(func, = SQL_FUNC_UNLIKELY)) { > + if (nFarg < 1) { > + diag_set(ClientError, > + = ER_FUNC_WRONG_ARG_COUNT, > + func->def->name, > + ">=3D 1", nFarg); Nit: =E2=80=9Cat least one=E2=80=9D > + const char *name; > + /** Members below are related to struct func_sql_builtin. */ > + uint16_t flags; > + void (*call)(sql_context *ctx, int argc, sql_value **argv); > + void (*finalize)(sql_context *ctx); > + /** Members below are related to struct func_def. */ > + bool is_deterministic; > + int param_count; > + enum field_type returns; > + enum func_aggregate aggregate; > + bool export_to_sql; > +} sql_builtins[] =3D { > + {.name =3D "ABS", > + .param_count =3D 1, > + .returns =3D FIELD_TYPE_NUMBER, > + .aggregate =3D FUNC_AGGREGATE_NONE, > + .is_deterministic =3D true, > + .flags =3D 0, > + .call =3D absFunc, > + .finalize =3D NULL, > + .export_to_sql =3D true, > + }, { > + .name =3D "AVG", > + .param_count =3D 1, > + .returns =3D FIELD_TYPE_NUMBER, > + .is_deterministic =3D false, > + .aggregate =3D FUNC_AGGREGATE_GROUP, > + .flags =3D 0, > + .call =3D sum_step, > + .finalize =3D avgFinalize, > + .export_to_sql =3D true, > + }, { > + .name =3D "CEIL", > + .call =3D sql_builtin_stub, > + .export_to_sql =3D false, >=20 Nit: personally I=E2=80=99d not skip members and fill in them all. > +struct func * > +func_sql_builtin_new(struct func_def *def) > +{ > + assert(def->language =3D=3D FUNC_LANGUAGE_SQL_BUILTIN); > + /** Binary search for corresponding builtin entry. */ > + int idx =3D -1, left =3D 0, right =3D nelem(sql_builtins) - 1; > + while (left <=3D right) { > + uint32_t mid =3D (left + right) / 2; > + int rc =3D strcmp(def->name, sql_builtins[mid].name); > + if (rc =3D=3D 0) { > + idx =3D mid; > + break; > } > + if (rc < 0) > + right =3D mid - 1; > + else > + left =3D mid + 1; > } > -#endif > + /* Some builtins are not implemented yet. */ Please, left comment describing why we really do need this check. I mean the fact that it disallows user to create random built-in = functions. > diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c > index 83dff47b6..231c670f5 100644 > --- a/src/box/sql/resolve.c > +++ b/src/box/sql/resolve.c > @@ -38,6 +38,7 @@ > #include "sqlInt.h" > #include > #include > +#include "box/schema.h" >=20 > /* > * Walk the expression tree pExpr and increase the aggregate function > @@ -591,32 +592,46 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > case TK_FUNCTION:{ > ExprList *pList =3D pExpr->x.pList; /* The = argument list */ > int n =3D pList ? pList->nExpr : 0; /* = Number of arguments */ > - int no_such_func =3D 0; /* True if no such = function exists */ > - int wrong_num_args =3D 0; /* True if wrong = number of arguments */ > int is_agg =3D 0; /* True if is an = aggregate function */ > int nId; /* Number of characters in = function name */ > const char *zId; /* The function name. */ > - FuncDef *pDef; /* Information about the = function */ >=20 > assert(!ExprHasProperty(pExpr, EP_xIsSelect)); > zId =3D pExpr->u.zToken; > nId =3D sqlStrlen30(zId); > - pDef =3D sqlFindFunction(pParse->db, zId, n, 0); > - if (pDef =3D=3D 0) { > - pDef =3D > - sqlFindFunction(pParse->db, zId, = -2,0); > - if (pDef =3D=3D 0) { > - no_such_func =3D 1; > + struct func *func =3D sql_func_by_signature(zId, = n); > + if (func =3D=3D NULL) { > + func =3D func_by_name(zId, nId); > + if (func =3D=3D NULL) { > + diag_set(ClientError, > + ER_NO_SUCH_FUNCTION, = zId); > + } else if (!func->def->exports.sql) { > + diag_set(ClientError, > + ER_SQL_PARSER_GENERIC, > + tt_sprintf("function = %.*s() " > + "is not = available " > + "in SQL", = nId, zId)); > } else { Let=E2=80=99s avoid call of sql_func_by_signature(). Consider = refactoring: diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c index 231c670f5..e7cf22cb6 100644 --- a/src/box/sql/resolve.c +++ b/src/box/sql/resolve.c @@ -599,30 +599,32 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) assert(!ExprHasProperty(pExpr, EP_xIsSelect)); zId =3D pExpr->u.zToken; nId =3D sqlStrlen30(zId); - struct func *func =3D sql_func_by_signature(zId, = n); + struct func *func =3D func_by_name(zId, nId); if (func =3D=3D NULL) { - func =3D func_by_name(zId, nId); - if (func =3D=3D NULL) { - diag_set(ClientError, - ER_NO_SUCH_FUNCTION, = zId); - } else if (!func->def->exports.sql) { - diag_set(ClientError, - ER_SQL_PARSER_GENERIC, - tt_sprintf("function = %.*s() " - "is not = available " - "in SQL", = nId, zId)); - } else { - uint32_t argc =3D = func->def->param_count; - const char *err =3D = tt_sprintf("%d", argc); - diag_set(ClientError, - = ER_FUNC_WRONG_ARG_COUNT, - func->def->name, err, = n); - } + diag_set(ClientError, = ER_NO_SUCH_FUNCTION, zId); pParse->is_aborted =3D true; pNC->nErr++; - } else { - is_agg =3D func->def->aggregate =3D=3D - FUNC_AGGREGATE_GROUP; + return WRC_Abort; + } + if (!func->def->exports.sql) { + diag_set(ClientError, = ER_SQL_PARSER_GENERIC, + tt_sprintf("function %.*s() is = not " + "available in SQL", = nId, + zId)); + pParse->is_aborted =3D true; + pNC->nErr++; + return WRC_Abort; + } + if (func->def->param_count !=3D n) { + uint32_t argc =3D = func->def->param_count; + const char *err =3D tt_sprintf("%d", = argc); + diag_set(ClientError, = ER_FUNC_WRONG_ARG_COUNT, + func->def->name, err, n); + pParse->is_aborted =3D true; + pNC->nErr++; + return WRC_Abort; + } etc > diff --git a/test/box/function1.test.lua b/test/box/function1.test.lua > index f894472f8..665972eda 100644 > --- a/test/box/function1.test.lua > +++ b/test/box/function1.test.lua > @@ -294,6 +294,13 @@ ok =3D=3D true >=20 > box.func.LUA:call({"return 1 + 1"}) >=20 > +box.schema.user.grant('guest', 'execute', 'function', 'SUM') > +c =3D net.connect(box.cfg.listen) > +c:call("SUM") > +c:close() > +box.schema.user.revoke('guest', 'execute', 'function', 'SUM') > +box.schema.func.drop("SUM=E2=80=9D) Didn=E2=80=99t forget check that user can=E2=80=99t create manually = built-ins?