[Tarantool-patches] [PATCH v1 5/7] sql: check built-in functions argument types

[imeevma@tarantool.org]

This patch creates a uniform way to check the argument types of SQL
built-in functions. Prior to this patch, argument types were checked
inside functions. They are now checked in most cases in the ApplyType
opcode.

The only case where there is additional validation in a function is when
a function can take either STRING or VARBINARY as an argument. In this
case, the definition says that the function accepts a SCALAR, but in
fact only STRING or VARBINARY should be accepted. This case will be
completely fixed in the next patches of the patch set.

Part of #4159
---
 src/box/sql/expr.c         |  4 ++++
 src/box/sql/select.c       | 26 ++++++++++++++++++++++++++
 src/box/sql/sqlInt.h       | 14 ++++++++++++++
 test/sql-tap/func.test.lua | 22 +++++++++++-----------
 test/sql/boolean.result    |  2 +-
 test/sql/checks.result     |  8 --------
 test/sql/checks.test.lua   |  2 --
 test/sql/types.result      |  2 +-
 8 files changed, 57 insertions(+), 23 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index bc2182446..e2c8e1385 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4120,6 +4120,10 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 			} else {
 				r1 = 0;
 			}
+			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
+				sql_emit_func_arg_type_check(v, func, r1,
+							     nFarg);
+			}
 			if (sql_func_flag_is_set(func, SQL_FUNC_NEEDCOLL)) {
 				sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0,
 						  (char *)coll, P4_COLLSEQ);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index b0554a172..49f01eb0d 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -124,6 +124,31 @@ clearSelect(sql * db, Select * p, int bFree)
 	}
 }
 
+void
+sql_emit_func_arg_type_check(struct Vdbe *vdbe, struct func *func, int reg,
+			     uint32_t argc)
+{
+	if (argc == 0 || func->def->param_list == NULL)
+		return;
+	assert(func->def->param_count > 0);
+	uint32_t len = (uint32_t)func->def->param_count;
+	assert(len > 0);
+	size_t size = (argc + 1) * sizeof(enum field_type);
+	enum field_type *types = sqlDbMallocZero(sql_get(), size);
+	if (argc <= len) {
+		for (uint32_t i = 0; i < argc; ++i)
+			types[i] = func->def->param_list[i];
+	} else {
+		for (uint32_t i = 0; i < len; ++i)
+			types[i] = func->def->param_list[i];
+		for (uint32_t i = len; i < argc; ++i)
+			types[i] = func->def->param_list[len - 1];
+	}
+	types[argc] = field_type_MAX;
+	sqlVdbeAddOp4(vdbe, OP_ApplyType, reg, argc, 0, (char *)types,
+		      P4_DYNAMIC);
+}
+
 /*
  * Initialize a SelectDest structure.
  */
@@ -5420,6 +5445,7 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 			vdbe_insert_distinct(pParse, pF->iDistinct, pF->reg_eph,
 					     addrNext, 1, regAgg);
 		}
+		sql_emit_func_arg_type_check(v, pF->func, regAgg, nArg);
 		if (sql_func_flag_is_set(pF->func, SQL_FUNC_NEEDCOLL)) {
 			struct coll *coll = NULL;
 			struct ExprList_item *pItem;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index adf90d824..d02614140 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -3883,6 +3883,20 @@ sql_index_type_str(struct sql *db, const struct index_def *idx_def);
 void
 sql_emit_table_types(struct Vdbe *v, struct space_def *def, int reg);
 
+/**
+ * Code an OP_ApplyType opcode that try to cast implicitly types
+ * for given range of register starting from @a reg. These values
+ * then will be used as arguments of a function.
+ *
+ * @param vdbe VDBE.
+ * @param func Definition of the function.
+ * @param reg Register where types will be placed.
+ * @param argc Number of arguments.
+ */
+void
+sql_emit_func_arg_type_check(struct Vdbe *vdbe, struct func *func,
+			     int reg, uint32_t argc);
+
 enum field_type
 sql_type_result(enum field_type lhs, enum field_type rhs);
 
diff --git a/test/sql-tap/func.test.lua b/test/sql-tap/func.test.lua
index 3c088920f..82cf350ea 100755
--- a/test/sql-tap/func.test.lua
+++ b/test/sql-tap/func.test.lua
@@ -412,13 +412,13 @@ test:do_execsql_test(
         -- </func-4.4.1>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "func-4.4.2",
     [[
         SELECT abs(t1) FROM tbl1
     ]], {
         -- <func-4.4.2>
-        0.0, 0.0, 0.0, 0.0, 0.0
+        1, "Type mismatch: can not convert this to number"
         -- </func-4.4.2>
     })
 
@@ -502,13 +502,13 @@ test:do_execsql_test(
         -- </func-4.12>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "func-4.13",
     [[
         SELECT round(t1,2) FROM tbl1
     ]], {
         -- <func-4.13>
-        0.0, 0.0, 0.0, 0.0, 0.0
+        1, "Type mismatch: can not convert this to double"
         -- </func-4.13>
     })
 
@@ -893,7 +893,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-8.5",
     [[
-        SELECT sum(x) FROM (SELECT '9223372036' || '854775807' AS x
+        SELECT sum(x) FROM (SELECT CAST('9223372036' || '854775807' AS INTEGER) AS x
                             UNION ALL SELECT -9223372036854775807)
     ]], {
         -- <func-8.5>
@@ -904,7 +904,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-8.6",
     [[
-        SELECT typeof(sum(x)) FROM (SELECT '9223372036' || '854775807' AS x
+        SELECT typeof(sum(x)) FROM (SELECT CAST('9223372036' || '854775807' AS INTEGER) AS x
                             UNION ALL SELECT -9223372036854775807)
     ]], {
         -- <func-8.6>
@@ -915,7 +915,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-8.7",
     [[
-        SELECT typeof(sum(x)) FROM (SELECT '9223372036' || '854775808' AS x
+        SELECT typeof(sum(x)) FROM (SELECT CAST('9223372036' || '854775808' AS INTEGER) AS x
                             UNION ALL SELECT -9223372036854775807)
     ]], {
         -- <func-8.7>
@@ -926,7 +926,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-8.8",
     [[
-        SELECT sum(x)>0.0 FROM (SELECT '9223372036' || '854775808' AS x
+        SELECT sum(x)>0.0 FROM (SELECT CAST('9223372036' || '854775808' AS INTEGER) AS x
                             UNION ALL SELECT -9223372036850000000)
     ]], {
         -- <func-8.8>
@@ -985,7 +985,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "func-9.5",
     [[
-        SELECT length(randomblob(32)), length(randomblob(-5)),
+        SELECT length(randomblob(32)), length(randomblob(0)),
                length(randomblob(2000))
     ]], {
         -- <func-9.5>
@@ -2918,7 +2918,7 @@ test:do_catchsql_test(
         SELECT ROUND(X'FF')
     ]], {
         -- <func-76.1>
-        1, "Type mismatch: can not convert varbinary to numeric"
+        1, "Type mismatch: can not convert varbinary to double"
         -- </func-76.1>
     })
 
@@ -2928,7 +2928,7 @@ test:do_catchsql_test(
         SELECT RANDOMBLOB(X'FF')
     ]], {
         -- <func-76.2>
-        1, "Type mismatch: can not convert varbinary to numeric"
+        1, "Type mismatch: can not convert varbinary to unsigned"
         -- </func-76.2>
     })
 
diff --git a/test/sql/boolean.result b/test/sql/boolean.result
index 51ec5820b..d366aca7d 100644
--- a/test/sql/boolean.result
+++ b/test/sql/boolean.result
@@ -276,7 +276,7 @@ SELECT is_boolean('true');
 SELECT abs(a) FROM t0;
  | ---
  | - null
- | - 'Inconsistent types: expected number got boolean'
+ | - 'Type mismatch: can not convert FALSE to number'
  | ...
 SELECT lower(a) FROM t0;
  | ---
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 7b18e5d6b..3c942fb23 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -519,14 +519,6 @@ s:insert({1, 'string'})
 ---
 - error: 'Check constraint failed ''complex2'': typeof(coalesce(z,0))==''integer'''
 ...
-s:insert({1, {map=true}})
----
-- error: 'Check constraint failed ''complex2'': typeof(coalesce(z,0))==''integer'''
-...
-s:insert({1, {'a', 'r','r','a','y'}})
----
-- error: 'Check constraint failed ''complex2'': typeof(coalesce(z,0))==''integer'''
-...
 s:insert({1, 3.14})
 ---
 - error: 'Check constraint failed ''complex2'': typeof(coalesce(z,0))==''integer'''
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index 301f8ea69..b55abe955 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -173,8 +173,6 @@ s:format({{name='X', type='integer'}, {name='Z', type='any'}})
 _ = s:create_index('pk', {parts = {1, 'integer'}})
 _ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\'', true})
 s:insert({1, 'string'})
-s:insert({1, {map=true}})
-s:insert({1, {'a', 'r','r','a','y'}})
 s:insert({1, 3.14})
 s:insert({1, 666})
 s:drop()
diff --git a/test/sql/types.result b/test/sql/types.result
index 2498f3a48..82a82117b 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1322,7 +1322,7 @@ box.execute("SELECT upper(v) FROM t;")
 box.execute("SELECT abs(v) FROM t;")
 ---
 - null
-- 'Inconsistent types: expected number got varbinary'
+- 'Type mismatch: can not convert varbinary to number'
 ...
 box.execute("SELECT typeof(v) FROM t;")
 ---
-- 
2.25.1