[tarantool-patches] [PATCH 2/9] sql: disallow text values participate in sum() aggregate

[Nikita Pettik <korablev@tarantool.org>]

It is obvious that we can't add string with number except the case when
string is a number literal in quotes (aka '123' or '0.5'). Before this
patch values which can't be converted to numbers were just skipped.
Now error is raised. Another one misbehavior was in using
sql_value_numeric_type() function, which set flag indicating number
value in memory cell, but didn't clear MEM_Str flag. As a result, we
couldn't determine type of value in such memory cell without
ambigiousness.
---
 src/box/sql/func.c            | 38 ++++++++++++++++++++++++--------------
 src/box/sql/sqlInt.h          |  3 ---
 src/box/sql/vdbe.c            | 19 ++-----------------
 src/box/sql/vdbeInt.h         |  3 +--
 test/sql-tap/select1.test.lua |  4 ++--
 test/sql-tap/select5.test.lua |  3 ++-
 6 files changed, 31 insertions(+), 39 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b86a95d9a..9adfeec67 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1495,24 +1495,34 @@ static void
 sumStep(sql_context * context, int argc, sql_value ** argv)
 {
 	SumCtx *p;
-	int type;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
 	p = sql_aggregate_context(context, sizeof(*p));
-	type = sql_value_numeric_type(argv[0]);
-	if (p && type != SQL_NULL) {
-		p->cnt++;
-		if (type == SQL_INTEGER) {
-			i64 v = sql_value_int64(argv[0]);
-			p->rSum += v;
-			if ((p->approx | p->overflow) == 0
-			    && sqlAddInt64(&p->iSum, v)) {
-				p->overflow = 1;
-			}
-		} else {
-			p->rSum += sql_value_double(argv[0]);
-			p->approx = 1;
+	assert(p != NULL);
+	int type = sql_value_type(argv[0]);
+	if (type == SQL_NULL)
+		return;
+	if (type != SQL_FLOAT && type != SQL_INTEGER) {
+		if (mem_apply_numeric_type(argv[0]) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 sql_value_text(argv[0]), "number");
+			context->fErrorOrAux = 1;
+			context->isError = SQL_TARANTOOL_ERROR;
+			return;
 		}
+		type = sql_value_type(argv[0]);
+	}
+	p->cnt++;
+	if (type == SQL_INTEGER) {
+		i64 v = sql_value_int64(argv[0]);
+		p->rSum += v;
+		if ((p->approx | p->overflow) == 0 &&
+		    sqlAddInt64(&p->iSum, v)) {
+			p->overflow = 1;
+		}
+	} else {
+		p->rSum += sql_value_double(argv[0]);
+		p->approx = 1;
 	}
 }
 
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index d8dc03284..b5c596d2f 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -458,9 +458,6 @@ sql_value_text(sql_value *);
 int
 sql_value_type(sql_value *);
 
-int
-sql_value_numeric_type(sql_value *);
-
 sql *
 sql_context_db_handle(sql_context *);
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 1b3e2a59d..c689aaf1d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -276,7 +276,8 @@ allocateCursor(
 int
 mem_apply_numeric_type(struct Mem *record)
 {
-	assert((record->flags & (MEM_Str | MEM_Int | MEM_Real)) == MEM_Str);
+	if ((record->flags & (MEM_Str | MEM_Int | MEM_Real)) != MEM_Str)
+		return -1;
 	int64_t integer_value;
 	if (sql_atoi64(record->z, &integer_value, record->n) == 0) {
 		record->u.i = integer_value;
@@ -355,22 +356,6 @@ mem_apply_type(struct Mem *record, enum field_type type)
 	}
 }
 
-/*
- * Try to convert the type of a function argument or a result column
- * into a numeric representation.  Use either INTEGER or REAL whichever
- * is appropriate.  But only do the conversion if it is possible without
- * loss of information and return the revised type of the argument.
- */
-int sql_value_numeric_type(sql_value *pVal) {
-	int eType = sql_value_type(pVal);
-	if (eType==SQL_TEXT) {
-		Mem *pMem = (Mem*)pVal;
-		mem_apply_numeric_type(pMem);
-		eType = sql_value_type(pVal);
-	}
-	return eType;
-}
-
 /*
  * Exported version of mem_apply_type(). This one works on sql_value*,
  * not the internal Mem* type.
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 8cd00d43a..ec9123a66 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -274,8 +274,7 @@ mem_type_to_str(const struct Mem *p);
  * if we can do so without loss of information. Firstly, value
  * is attempted to be converted to integer, and in case of fail -
  * to floating point number. Note that function is assumed to be
- * called only on memory cell containing string,
- * i.e. memory->type == MEM_Str.
+ * called on memory cell containing string, i.e. mem->type == MEM_Str.
  *
  * @param record Memory cell containing value to be converted.
  * @retval 0 If value can be converted to integer or number.
diff --git a/test/sql-tap/select1.test.lua b/test/sql-tap/select1.test.lua
index be63e815d..1bad7679f 100755
--- a/test/sql-tap/select1.test.lua
+++ b/test/sql-tap/select1.test.lua
@@ -503,13 +503,13 @@ test:do_catchsql_test(
         -- </select1-2.17>
     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "select1-2.17.1",
     [[
         SELECT sum(a) FROM t3
     ]], {
         -- <select1-2.17.1>
-        44.0
+        1, "Type mismatch: can not convert abc to number"
         -- </select1-2.17.1>
     })
 
diff --git a/test/sql-tap/select5.test.lua b/test/sql-tap/select5.test.lua
index 507448d00..1353b39fc 100755
--- a/test/sql-tap/select5.test.lua
+++ b/test/sql-tap/select5.test.lua
@@ -550,7 +550,7 @@ test:do_execsql_test(
     -- </select5-9.13>
 })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "select5-9.13.2",
     [[
             CREATE TABLE jj (s1 INT, s2 VARCHAR(1), PRIMARY KEY(s1));
@@ -558,6 +558,7 @@ test:do_execsql_test(
             SELECT 1 FROM jj HAVING avg(s2) = 1 AND avg(s2) = 0;
     ]], {
     -- <select5-9.13.2>
+    1, "Type mismatch: can not convert A to number"
     -- </select5-9.13.2>
 })
 
-- 
2.15.1