From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 16F364696C3 for ; Fri, 10 Apr 2020 13:39:48 +0300 (MSK) Date: Fri, 10 Apr 2020 13:39:46 +0300 From: Mergen Imeev Message-ID: <20200410103945.GA20019@tarantool.org> References: <01024a2279b5c030f272d2a97ce8bd83f8ababc3.1585308644.git.imeevma@gmail.com> <20200327164604.GC9287@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200327164604.GC9287@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for review. My answers and new patch below. On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote: > On 27 Mar 14:33, imeevma@tarantool.org wrote: > > Could you please find Peter's table containing current/expected cast > behaviours and verify that this patch doesn't contradict it? > > > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c > > index aad030d..de1d9c3 100644 > > --- a/src/box/sql/vdbemem.c > > +++ b/src/box/sql/vdbemem.c > > @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type) > > return -1; > > case FIELD_TYPE_INTEGER: > > case FIELD_TYPE_UNSIGNED: > > - if ((pMem->flags & MEM_Blob) != 0) { > > + if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) { > > bool is_neg; > > int64_t val; > > if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0) > > @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type) > > MemSetTypeFlag(pMem, MEM_UInt); > > return 0; > > } > > - if (sqlVdbeMemIntegerify(pMem) != 0) > > + if ((pMem->flags & MEM_Real) != 0) { > > + double d; > > + if (sqlVdbeRealValue(pMem, &d) != 0) > > + return -1; > > + if (d < INT64_MAX && d >= INT64_MIN) { > > + mem_set_int(pMem, d, d <= -1); > > + return 0; > > + } > > + if (d >= INT64_MAX && d < UINT64_MAX) { > > + mem_set_u64(pMem, d); > > + return 0; > > + } > > return -1; > > Instead of keeping inlining code into sqlVdbeMemCast() I'd better > split it into separate functions. Good refactoring task tho - just > keep in mind. > Thanks. I will fix this in #3809 issue. > > + } > > if (type == FIELD_TYPE_UNSIGNED && > > (pMem->flags & MEM_UInt) == 0) > > return -1; > > diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua > > new file mode 100755 > > index 0000000..0865c4e > > --- /dev/null > > +++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua > > Strictly speaking this change is not related to 4766, so I'd not > create separate test file for current patch. Moreover, tests in > sql/types.test.lua seem to cover it. > I removed this file. > > +#!/usr/bin/env tarantool > > +test = require("sqltester") > > +test:plan(6) > > + > > +-- > > +-- Make sure that STRING or BLOB that contains DOUBLE value cannot > > +-- be cast to INTEGER. > > +-- > > +test:do_catchsql_test( > > + "gh-4766-1", > > + [[ > > + SELECT CAST('1.1' AS INTEGER) > > + ]], { > > + 1, "Type mismatch: can not convert 1.1 to integer" > > + }) > > + > > +test:do_catchsql_test( > > + "gh-4766-2", > > + [[ > > + SELECT CAST(x'312e31' AS INTEGER) > > + ]], { > > + 1, "Type mismatch: can not convert varbinary to integer" > > + }) > > + > > diff --git a/test/sql/types.result b/test/sql/types.result > > index 38e4385..54aff46 100644 > > --- a/test/sql/types.result > > +++ b/test/sql/types.result > > @@ -269,11 +269,8 @@ box.space.T1:drop() > > -- > > box.execute("SELECT CAST('1.123' AS INTEGER);") > > --- > > -- metadata: > > - - name: CAST('1.123' AS INTEGER) > > - type: integer > > - rows: > > - - [1] > > +- null > > +- 'Type mismatch: can not convert 1.123 to integer' > > ... > > box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);") > > --- > > @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');") > > ... > > box.execute("SELECT CAST(f AS INTEGER) FROM t1;") > > --- > > -- metadata: > > - - name: CAST(f AS INTEGER) > > - type: integer > > - rows: > > - - [0] > > - - [1] > > - - [3] > > +- null > > +- 'Type mismatch: can not convert 0.0 to integer' > > ... > > box.space.T1:drop() > > --- > > @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);") > > ... > > box.execute("SELECT CAST(-1.5 AS UNSIGNED);") > > --- > > -- null > > -- 'Type mismatch: can not convert -1 to unsigned' > > +- metadata: > > + - name: CAST(-1.5 AS UNSIGNED) > > + type: unsigned > > + rows: > > + - [-1] > > ... > > box.execute("SELECT CAST(true AS UNSIGNED);") > > --- > > -- > > 2.7.4 > > New patch: >From d2679859651aeee2dffda01545e6c62ae3c185d1 Mon Sep 17 00:00:00 2001 From: Mergen Imeev Date: Mon, 16 Mar 2020 15:12:37 +0300 Subject: [PATCH] sql: fix CAST() from STRING to INTEGER Prior to this patch, STRING, which contains the DOUBLE value, could be cast to INTEGER. This was done by converting STRING to DOUBLE and then converting this DOUBLE value to INTEGER. This may affect the accuracy of CAST(), so it was forbidden. Before patch: box.execute("SELECT CAST('111.1' as INTEGER);") Result: 111 After patch: box.execute("SELECT CAST('1.1' as INTEGER);") Result: 'Type mismatch: can not convert 1.1 to integer' box.execute("SELECT CAST('1.0' as INTEGER);") Result: 'Type mismatch: can not convert 1.0 to integer' box.execute("SELECT CAST('1.' as INTEGER);") Result: 'Type mismatch: can not convert 1. to integer' diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index aad030d..de1d9c3 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type) return -1; case FIELD_TYPE_INTEGER: case FIELD_TYPE_UNSIGNED: - if ((pMem->flags & MEM_Blob) != 0) { + if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) { bool is_neg; int64_t val; if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0) @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type) MemSetTypeFlag(pMem, MEM_UInt); return 0; } - if (sqlVdbeMemIntegerify(pMem) != 0) + if ((pMem->flags & MEM_Real) != 0) { + double d; + if (sqlVdbeRealValue(pMem, &d) != 0) + return -1; + if (d < INT64_MAX && d >= INT64_MIN) { + mem_set_int(pMem, d, d <= -1); + return 0; + } + if (d >= INT64_MAX && d < UINT64_MAX) { + mem_set_u64(pMem, d); + return 0; + } return -1; + } if (type == FIELD_TYPE_UNSIGNED && (pMem->flags & MEM_UInt) == 0) return -1; diff --git a/test/sql/types.result b/test/sql/types.result index 38e4385..54aff46 100644 --- a/test/sql/types.result +++ b/test/sql/types.result @@ -269,11 +269,8 @@ box.space.T1:drop() -- box.execute("SELECT CAST('1.123' AS INTEGER);") --- -- metadata: - - name: CAST('1.123' AS INTEGER) - type: integer - rows: - - [1] +- null +- 'Type mismatch: can not convert 1.123 to integer' ... box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);") --- @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');") ... box.execute("SELECT CAST(f AS INTEGER) FROM t1;") --- -- metadata: - - name: CAST(f AS INTEGER) - type: integer - rows: - - [0] - - [1] - - [3] +- null +- 'Type mismatch: can not convert 0.0 to integer' ... box.space.T1:drop() --- @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);") ... box.execute("SELECT CAST(-1.5 AS UNSIGNED);") --- -- null -- 'Type mismatch: can not convert -1 to unsigned' +- metadata: + - name: CAST(-1.5 AS UNSIGNED) + type: unsigned + rows: + - [-1] ... box.execute("SELECT CAST(true AS UNSIGNED);") ---