From: "n.pettik" <korablev@tarantool.org> To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov <kshcherbatov@tarantool.org> Subject: [tarantool-patches] Re: [PATCH v4 3/4] sql: get rid of FuncDef function hash Date: Thu, 22 Aug 2019 17:37:39 +0300 [thread overview] Message-ID: <17C6E71F-24DF-476D-B5FA-430AF34DC84D@tarantool.org> (raw) In-Reply-To: <154ee8ea11ae260254b71d32e1299f1272438af1.1566400979.git.kshcherbatov@tarantool.org> > @@ -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; > > -/** 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’t exist yet… 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 = sqlVdbeMakeLabel(v); > - assert(nFarg >= 2); > + if (nFarg < 2) { > + diag_set(ClientError, > + ER_FUNC_WRONG_ARG_COUNT, > + func->def->name, > + ">= 2", nFarg); -> “more than one”/“at least two” What is more, you can move introduction of ER_FUNC_WRONG_… 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 >= 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, > + ">= 1", nFarg); Nit: “at least one” > + 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[] = { > + {.name = "ABS", > + .param_count = 1, > + .returns = FIELD_TYPE_NUMBER, > + .aggregate = FUNC_AGGREGATE_NONE, > + .is_deterministic = true, > + .flags = 0, > + .call = absFunc, > + .finalize = NULL, > + .export_to_sql = true, > + }, { > + .name = "AVG", > + .param_count = 1, > + .returns = FIELD_TYPE_NUMBER, > + .is_deterministic = false, > + .aggregate = FUNC_AGGREGATE_GROUP, > + .flags = 0, > + .call = sum_step, > + .finalize = avgFinalize, > + .export_to_sql = true, > + }, { > + .name = "CEIL", > + .call = sql_builtin_stub, > + .export_to_sql = false, > Nit: personally I’d not skip members and fill in them all. > +struct func * > +func_sql_builtin_new(struct func_def *def) > +{ > + assert(def->language == FUNC_LANGUAGE_SQL_BUILTIN); > + /** Binary search for corresponding builtin entry. */ > + int idx = -1, left = 0, right = nelem(sql_builtins) - 1; > + while (left <= right) { > + uint32_t mid = (left + right) / 2; > + int rc = strcmp(def->name, sql_builtins[mid].name); > + if (rc == 0) { > + idx = mid; > + break; > } > + if (rc < 0) > + right = mid - 1; > + else > + left = 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 <stdlib.h> > #include <string.h> > +#include "box/schema.h" > > /* > * Walk the expression tree pExpr and increase the aggregate function > @@ -591,32 +592,46 @@ resolveExprStep(Walker * pWalker, Expr * pExpr) > case TK_FUNCTION:{ > ExprList *pList = pExpr->x.pList; /* The argument list */ > int n = pList ? pList->nExpr : 0; /* Number of arguments */ > - int no_such_func = 0; /* True if no such function exists */ > - int wrong_num_args = 0; /* True if wrong number of arguments */ > int is_agg = 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 */ > > assert(!ExprHasProperty(pExpr, EP_xIsSelect)); > zId = pExpr->u.zToken; > nId = sqlStrlen30(zId); > - pDef = sqlFindFunction(pParse->db, zId, n, 0); > - if (pDef == 0) { > - pDef = > - sqlFindFunction(pParse->db, zId, -2,0); > - if (pDef == 0) { > - no_such_func = 1; > + struct func *func = sql_func_by_signature(zId, n); > + if (func == NULL) { > + func = func_by_name(zId, nId); > + if (func == 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’s 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 = pExpr->u.zToken; nId = sqlStrlen30(zId); - struct func *func = sql_func_by_signature(zId, n); + struct func *func = func_by_name(zId, nId); if (func == NULL) { - func = func_by_name(zId, nId); - if (func == 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 = func->def->param_count; - const char *err = 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 = true; pNC->nErr++; - } else { - is_agg = func->def->aggregate == - 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 = true; + pNC->nErr++; + return WRC_Abort; + } + if (func->def->param_count != n) { + uint32_t argc = func->def->param_count; + const char *err = tt_sprintf("%d", argc); + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, + func->def->name, err, n); + pParse->is_aborted = 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 == true > > box.func.LUA:call({"return 1 + 1"}) > > +box.schema.user.grant('guest', 'execute', 'function', 'SUM') > +c = net.connect(box.cfg.listen) > +c:call("SUM") > +c:close() > +box.schema.user.revoke('guest', 'execute', 'function', 'SUM') > +box.schema.func.drop("SUM”) Didn’t forget check that user can’t create manually built-ins?
next prev parent reply other threads:[~2019-08-22 14:37 UTC|newest] Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-08-21 15:28 [tarantool-patches] [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem Kirill Shcherbatov 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 1/4] sql: rename sql_vdbe_mem_alloc_region helper Kirill Shcherbatov 2019-08-22 13:04 ` [tarantool-patches] " n.pettik 2019-08-23 15:02 ` Kirill Shcherbatov 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 2/4] sql: replace flag MINMAX with flags MIN and MAX Kirill Shcherbatov 2019-08-22 13:30 ` [tarantool-patches] " n.pettik 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 3/4] sql: get rid of FuncDef function hash Kirill Shcherbatov 2019-08-22 14:37 ` n.pettik [this message] 2019-08-23 15:02 ` [tarantool-patches] [PATCH v4 4/5] " Kirill Shcherbatov 2019-08-23 15:02 ` [tarantool-patches] [PATCH v4 3/5] sql: remove name overloading for SQL builtins Kirill Shcherbatov 2019-08-28 15:05 ` [tarantool-patches] " Nikita Pettik 2019-08-23 15:02 ` [tarantool-patches] Re: [PATCH v4 3/4] sql: get rid of FuncDef function hash Kirill Shcherbatov 2019-08-21 15:28 ` [tarantool-patches] [PATCH v4 4/4] sql: support user-defined functions in SQL Kirill Shcherbatov 2019-08-22 15:23 ` [tarantool-patches] " n.pettik 2019-08-23 15:02 ` Kirill Shcherbatov 2019-08-29 15:09 ` [tarantool-patches] Re: [PATCH v4 0/4] sql: uniform SQL and Lua functions subsystem Nikita Pettik 2019-08-29 17:12 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=17C6E71F-24DF-476D-B5FA-430AF34DC84D@tarantool.org \ --to=korablev@tarantool.org \ --cc=kshcherbatov@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH v4 3/4] sql: get rid of FuncDef function hash' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox