Re: [Tarantool-patches] [PATCH v2 2/6] sql: introduce sql_func_find()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the review! My answer, diff and new patch below.

On Fri, Aug 06, 2021 at 12:15:20AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 28d383293..9514d070a 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -2655,6 +2643,30 @@ static struct {
> >  	},
> >  };
> >  
> > +struct func *
> > +sql_func_find(struct Expr *expr)
> > +{
> > +	const char *name = expr->u.zToken;
> > +	struct func *func = func_by_name(name, strlen(name));
> > +	if (func == NULL) {
> > +		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
> > +		return NULL;
> > +	}
> > +	if (!func->def->exports.sql) {
> > +		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
> > +			 tt_sprintf("function %s() is not available in SQL",
> > +				     name));
> > +		return NULL;
> > +	}
> > +	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
> 
> != NULL.
> 
Fixed.

> > +	if (func->def->param_count != -1 && func->def->param_count != n) {
> > +		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
> > +			 tt_sprintf("%d", func->def->param_count), n);
> > +		return NULL;
> > +	}
> > +	return func;
> > +}

Diff:


diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9514d070a..7cdcce6bc 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2658,7 +2658,7 @@ sql_func_find(struct Expr *expr)
 				     name));
 		return NULL;
 	}
-	int n = expr->x.pList ? expr->x.pList->nExpr : 0;
+	int n = expr->x.pList != NULL ? expr->x.pList->nExpr : 0;
 	if (func->def->param_count != -1 && func->def->param_count != n) {
 		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
 			 tt_sprintf("%d", func->def->param_count), n);



New patch:


commit fbb57e6bf3a40d387a1641f4233680613d3b7f5e
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Jul 31 19:05:05 2021 +0300

    sql: introduce sql_func_find()
    
    This patch introduces the sql_func_find() function. This function allows
    us to centralize the look up of functions during parsing, which
    simplifies code and fixes some incorrect error messages.
    
    Part of #6106

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index b87f69512..afa2331a1 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -719,6 +719,10 @@ callStatGet(Vdbe * v, int regStat4, int iParam, int regOut)
 {
 	assert(regOut != regStat4 && regOut != regStat4 + 1);
 	sqlVdbeAddOp2(v, OP_Integer, iParam, regStat4 + 1);
+	/*
+	 * Function sql_func_by_signature() was removed, so after enabling this
+	 * part should be changed.
+	 */
 	struct func *func = sql_func_by_signature("_sql_stat_get", 2);
 	assert(func != NULL);
 	sqlVdbeAddOp4(v, OP_BuiltinFunction0, 0, regStat4, regOut,
@@ -858,6 +862,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		sqlVdbeAddOp2(v, OP_Count, idx_cursor, stat4_reg + 3);
 		sqlVdbeAddOp2(v, OP_Integer, part_count, stat4_reg + 1);
 		sqlVdbeAddOp2(v, OP_Integer, part_count, stat4_reg + 2);
+		/*
+		 * Function sql_func_by_signature() was removed, so after
+		 * enabling this part should be changed.
+		 */
 		struct func *init_func =
 			sql_func_by_signature("_sql_stat_init", 3);
 		assert(init_func != NULL);
@@ -959,6 +967,10 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		sqlVdbeAddOp3(v, OP_MakeRecord, stat_key_reg,
 				  pk_part_count, key_reg);
 		assert(chng_reg == (stat4_reg + 1));
+		/*
+		 * Function sql_func_by_signature() was removed, so after
+		 * enabling this part should be changed.
+		 */
 		struct func *push_func =
 			sql_func_by_signature("_sql_stat_push", 3);
 		assert(push_func != NULL);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 80f2d349a..20d22455c 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3957,7 +3957,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 	case TK_FUNCTION:{
 			ExprList *pFarg;	/* List of function arguments */
 			int nFarg;	/* Number of function arguments */
-			const char *zId;	/* The function name */
 			u32 constMask = 0;	/* Mask of function arguments that are constant */
 			int i;	/* Loop counter */
 			struct coll *coll = NULL;
@@ -3970,11 +3969,8 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			}
 			nFarg = pFarg ? pFarg->nExpr : 0;
 			assert(!ExprHasProperty(pExpr, EP_IntValue));
-			zId = pExpr->u.zToken;
-			struct func *func = sql_func_by_signature(zId, nFarg);
+			struct func *func = sql_func_find(pExpr);
 			if (func == NULL) {
-				diag_set(ClientError, ER_NO_SUCH_FUNCTION,
-					 zId);
 				pParse->is_aborted = true;
 				break;
 			}
@@ -5431,14 +5427,8 @@ analyzeAggregate(Walker * pWalker, Expr * pExpr)
 						pItem->iMem = ++pParse->nMem;
 						assert(!ExprHasProperty
 						       (pExpr, EP_IntValue));
-						const char *name =
-							pExpr->u.zToken;
-						uint32_t argc =
-							pExpr->x.pList != NULL ?
-							pExpr->x.pList->nExpr : 0;
 						pItem->func =
-							sql_func_by_signature(
-								name, argc);
+							sql_func_find(pExpr);
 						assert(pItem->func != NULL);
 						assert(pItem->func->def->
 						       language ==
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 28d383293..7cdcce6bc 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1934,24 +1934,12 @@ sql_is_like_func(struct Expr *expr)
 	    expr->x.pList->nExpr != 2)
 		return 0;
 	assert(!ExprHasProperty(expr, EP_xIsSelect));
-	struct func *func = sql_func_by_signature(expr->u.zToken, 2);
+	struct func *func = sql_func_find(expr);
 	if (func == NULL || !sql_func_flag_is_set(func, SQL_FUNC_LIKE))
 		return 0;
 	return 1;
 }
 
-struct func *
-sql_func_by_signature(const char *name, int argc)
-{
-	struct func *base = func_by_name(name, strlen(name));
-	if (base == NULL || !base->def->exports.sql)
-		return NULL;
-
-	if (base->def->param_count != -1 && base->def->param_count != argc)
-		return NULL;
-	return base;
-}
-
 static int
 func_sql_builtin_call_stub(struct func *func, struct port *args,
 			   struct port *ret)
@@ -2655,6 +2643,30 @@ static struct {
 	},
 };
 
+struct func *
+sql_func_find(struct Expr *expr)
+{
+	const char *name = expr->u.zToken;
+	struct func *func = func_by_name(name, strlen(name));
+	if (func == NULL) {
+		diag_set(ClientError, ER_NO_SUCH_FUNCTION, name);
+		return NULL;
+	}
+	if (!func->def->exports.sql) {
+		diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+			 tt_sprintf("function %s() is not available in SQL",
+				     name));
+		return NULL;
+	}
+	int n = expr->x.pList != NULL ? expr->x.pList->nExpr : 0;
+	if (func->def->param_count != -1 && func->def->param_count != n) {
+		diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, name,
+			 tt_sprintf("%d", func->def->param_count), n);
+		return NULL;
+	}
+	return func;
+}
+
 uint32_t
 sql_func_flags(const char *name)
 {
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 11b6139e3..35faddab5 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -598,28 +598,8 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
 			assert(!ExprHasProperty(pExpr, EP_xIsSelect));
 			zId = pExpr->u.zToken;
 			nId = sqlStrlen30(zId);
-			struct func *func = func_by_name(zId, nId);
+			struct func *func = sql_func_find(pExpr);
 			if (func == NULL) {
-				diag_set(ClientError, ER_NO_SUCH_FUNCTION, zId);
-				pParse->is_aborted = true;
-				pNC->nErr++;
-				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 != -1 &&
-			    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;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index a92de0a2f..c6927e1e4 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4362,17 +4362,9 @@ sql_func_flag_is_set(struct func *func, uint16_t flag)
 	return (((struct func_sql_builtin *)func)->flags & flag) != 0;
 }
 
-/**
- * A SQL method to find a function in a hash by its name and
- * count of arguments. Only functions that have 'SQL' engine
- * export field set true and have exactly the same signature
- * are returned.
- *
- * Returns not NULL function pointer when a valid and exported
- * to SQL engine function is found and NULL otherwise.
- */
+/** Return a function that matches the parameters described in given expr. */
 struct func *
-sql_func_by_signature(const char *name, int argc);
+sql_func_find(struct Expr *expr);
 
 /**
  * Return the parameters of the function with the given name. If the function
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2c5099616..499089c8d 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -148,7 +148,7 @@ valueFromFunction(sql * db,	/* The database connection */
 	pList = p->x.pList;
 	if (pList)
 		nVal = pList->nExpr;
-	struct func *func = sql_func_by_signature(p->u.zToken, nVal);
+	struct func *func = sql_func_find(p);
 	if (func == NULL || func->def->language != FUNC_LANGUAGE_SQL_BUILTIN ||
 	    !func->def->is_deterministic ||
 	    sql_func_flag_is_set(func, SQL_FUNC_NEEDCOLL))
diff --git a/test/sql-tap/func5.test.lua b/test/sql-tap/func5.test.lua
index 9b1526aaf..13698582b 100755
--- a/test/sql-tap/func5.test.lua
+++ b/test/sql-tap/func5.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 local test = require("sqltester")
-test:plan(25)
+test:plan(27)
 
 --!./tcltestrunner.lua
 -- 2010 August 27
@@ -314,4 +314,36 @@ test:do_execsql_test(
 box.func.COUNTER1:drop()
 box.func.COUNTER2:drop()
 
+--
+-- Make sure the correct error is displayed if the function throws an error when
+-- setting the default value.
+--
+local body = 'function(x) return 1 end'
+box.schema.func.create('F1', {language = 'Lua', returns = 'number', body = body,
+                       param_list = {}, exports = {'LUA'}});
+box.execute([[CREATE TABLE t01(i INT PRIMARY KEY, a INT DEFAULT(f1(1)));]])
+test:do_catchsql_test(
+    "func-7.1",
+    [[
+        INSERT INTO t01(i) VALUES(1);
+    ]], {
+        1, "function F1() is not available in SQL"
+    })
+
+box.schema.func.create('F2', {language = 'Lua', returns = 'number', body = body,
+                       exports = {'LUA', 'SQL'}});
+box.execute([[CREATE TABLE t02(i INT PRIMARY KEY, a INT DEFAULT(f2(1)));]])
+test:do_catchsql_test(
+    "func-7.2",
+    [[
+        INSERT INTO t02(i) VALUES(1);
+    ]], {
+        1, "Wrong number of arguments is passed to F2(): expected 0, got 1"
+    })
+
+box.func.F1:drop()
+box.func.F2:drop()
+box.space.T01:drop()
+box.space.T02:drop()
+
 test:finish_test()