[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