From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 02AAB2BBD7 for ; Sun, 14 Apr 2019 11:04:14 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nG7EUc63oWeh for ; Sun, 14 Apr 2019 11:04:13 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 0569F2BB33 for ; Sun, 14 Apr 2019 11:04:13 -0400 (EDT) From: Nikita Pettik Subject: [tarantool-patches] [PATCH 2/9] sql: disallow text values participate in sum() aggregate Date: Sun, 14 Apr 2019 18:04:00 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: v.shpilevoy@tarantool.org, kostja@tarantool.org, Nikita Pettik 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( -- }) -test:do_execsql_test( +test:do_catchsql_test( "select1-2.17.1", [[ SELECT sum(a) FROM t3 ]], { -- - 44.0 + 1, "Type mismatch: can not convert abc to number" -- }) 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( -- }) -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; ]], { -- + 1, "Type mismatch: can not convert A to number" -- }) -- 2.15.1