[tarantool-patches] Re: [PATCH v4 3/4] sql: get rid of FuncDef function hash
n.pettik
korablev at tarantool.org
Thu Aug 22 17:37:39 MSK 2019
> @@ -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?
More information about the Tarantool-patches
mailing list