* [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER @ 2020-03-27 11:33 imeevma 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw) To: korablev, tsafin, tarantool-patches This patch-set fixes CAST() from BLOB to INTEGER in case a BLOB does not have '\0'. https://github.com/tarantool/tarantool/issues/4766 https://github.com/tarantool/tarantool/tree/imeevma/gh-4766-fix-blob-size-for-cast @Changelog - Explicit and implicit cast from string contains DOUBLE value to INTEGER or UNSIGNED was disallowed. - Maximum length of a BLOB that is allowed to be cast to INTEGER or UNSIGNED was limited to 12287 bytes. - Fixed wrong behaviour of CAST() from BLOB to INTEGER in case a BLOB does not have '\0'. (gh-4766) Mergen Imeev (3): sql: fix CAST() from STRING to INTEGER sql: fix implicit cast from STRING to INTEGER sql: add '\0' to the BLOB when it is cast to INTEGER src/box/sql/util.c | 17 ++- src/box/sql/vdbe.c | 11 +- src/box/sql/vdbeInt.h | 1 - src/box/sql/vdbemem.c | 45 ++----- test/sql-tap/e_select1.test.lua | 2 +- .../gh-4766-wrong-cast-from-blob-to-int.test.lua | 150 +++++++++++++++++++++ test/sql-tap/intpkey.test.lua | 2 +- test/sql-tap/join.test.lua | 4 +- test/sql-tap/subquery.test.lua | 6 +- test/sql-tap/tkt-9a8b09f8e6.test.lua | 4 +- test/sql/types.result | 23 ++-- 11 files changed, 203 insertions(+), 62 deletions(-) create mode 100755 test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua -- 2.7.4 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma @ 2020-03-27 11:33 ` imeevma 2020-03-27 16:46 ` Nikita Pettik 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw) To: korablev, tsafin, tarantool-patches 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' Part of #4766 --- src/box/sql/vdbemem.c | 16 +++++- .../gh-4766-wrong-cast-from-blob-to-int.test.lua | 57 ++++++++++++++++++++++ test/sql/types.result | 23 ++++----- 3 files changed, 80 insertions(+), 16 deletions(-) create mode 100755 test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua 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-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 @@ -0,0 +1,57 @@ +#!/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" + }) + +test:do_catchsql_test( + "gh-4766-3", + [[ + SELECT CAST('2.0' AS INTEGER) + ]], { + 1, "Type mismatch: can not convert 2.0 to integer" + }) + +test:do_catchsql_test( + "gh-4766-4", + [[ + SELECT CAST(x'322e30' AS INTEGER) + ]], { + 1, "Type mismatch: can not convert varbinary to integer" + }) + +test:do_catchsql_test( + "gh-4766-5", + [[ + SELECT CAST('2.' AS INTEGER) + ]], { + 1, "Type mismatch: can not convert 2. to integer" + }) + +test:do_catchsql_test( + "gh-4766-6", + [[ + SELECT CAST(x'322e' AS INTEGER) + ]], { + 1, "Type mismatch: can not convert varbinary to integer" + }) + +test:finish_test() 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma @ 2020-03-27 16:46 ` Nikita Pettik 2020-04-10 10:39 ` Mergen Imeev 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-03-27 16:46 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches 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. > + } > 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. > +#!/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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-03-27 16:46 ` Nikita Pettik @ 2020-04-10 10:39 ` Mergen Imeev 2020-04-10 10:43 ` Mergen Imeev 2020-04-10 12:46 ` Nikita Pettik 0 siblings, 2 replies; 18+ messages in thread From: Mergen Imeev @ 2020-04-10 10:39 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches 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 <imeevma@gmail.com> 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);") --- ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-04-10 10:39 ` Mergen Imeev @ 2020-04-10 10:43 ` Mergen Imeev 2020-04-10 13:05 ` Nikita Pettik 2020-04-10 12:46 ` Nikita Pettik 1 sibling, 1 reply; 18+ messages in thread From: Mergen Imeev @ 2020-04-10 10:43 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches Sorry, forgot to answer one question. On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote: > 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? > > Here: https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst ~ To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY --------------- ---------- ---------- --------- --------- ------------ From BOOLEAN | AAA | A-- | --- | A-- | --- From INTEGER | A-- | AAA | AAA | AAA | --- From NUMBER | A-- | SSA | AAA | AAA | --- From STRING | S-- | SSS | SSS | AAA | A-- From VARBINARY | --- | --- | --- | A-- | AAA Should be fine since we have S for CAST() and implicit cast from STRING 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; > > > > 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 <imeevma@gmail.com> > 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);") > --- ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-04-10 10:43 ` Mergen Imeev @ 2020-04-10 13:05 ` Nikita Pettik 2020-04-10 17:06 ` Imeev Mergen 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-04-10 13:05 UTC (permalink / raw) To: Mergen Imeev; +Cc: tarantool-patches On 10 Apr 13:43, Mergen Imeev wrote: > Sorry, forgot to answer one question. > > On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote: > > 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? > > > > Here: > https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst > > ~ To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY > --------------- ---------- ---------- --------- --------- ------------ > From BOOLEAN | AAA | A-- | --- | A-- | --- > From INTEGER | A-- | AAA | AAA | AAA | --- > From NUMBER | A-- | SSA | AAA | AAA | --- > From STRING | S-- | SSS | SSS | AAA | A-- > From VARBINARY | --- | --- | --- | A-- | AAA > > Should be fine since we have S for CAST() and implicit > cast from STRING to INTEGER. This table describes current behaviour. Could you also find the table which shows expected one? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-04-10 13:05 ` Nikita Pettik @ 2020-04-10 17:06 ` Imeev Mergen 2020-04-15 11:13 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Imeev Mergen @ 2020-04-10 17:06 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches On 4/10/20 4:05 PM, Nikita Pettik wrote: > On 10 Apr 13:43, Mergen Imeev wrote: >> Sorry, forgot to answer one question. >> >> On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote: >>> 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? >>>> >> Here: >> https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst >> >> ~ To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY >> --------------- ---------- ---------- --------- --------- ------------ >> From BOOLEAN | AAA | A-- | --- | A-- | --- >> From INTEGER | A-- | AAA | AAA | AAA | --- >> From NUMBER | A-- | SSA | AAA | AAA | --- >> From STRING | S-- | SSS | SSS | AAA | A-- >> From VARBINARY | --- | --- | --- | A-- | AAA >> >> Should be fine since we have S for CAST() and implicit >> cast from STRING to INTEGER. > This table describes current behaviour. Could you also find the table > which shows expected one? > I'm sorry, not sure that I have seen such table for CAST(). Should I ask Peter? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-04-10 17:06 ` Imeev Mergen @ 2020-04-15 11:13 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2020-04-15 11:13 UTC (permalink / raw) To: Imeev Mergen; +Cc: tarantool-patches On 10 Apr 20:06, Imeev Mergen wrote: > > On 4/10/20 4:05 PM, Nikita Pettik wrote: > > On 10 Apr 13:43, Mergen Imeev wrote: > > > Sorry, forgot to answer one question. > > > > > > On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote: > > > > 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? > > > > > > > > Here: > > > https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst > > > > > > ~ To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY > > > --------------- ---------- ---------- --------- --------- ------------ > > > From BOOLEAN | AAA | A-- | --- | A-- | --- > > > From INTEGER | A-- | AAA | AAA | AAA | --- > > > From NUMBER | A-- | SSA | AAA | AAA | --- > > > From STRING | S-- | SSS | SSS | AAA | A-- > > > From VARBINARY | --- | --- | --- | A-- | AAA > > > > > > Should be fine since we have S for CAST() and implicit > > > cast from STRING to INTEGER. > > This table describes current behaviour. Could you also find the table > > which shows expected one? > I'm sorry, not sure that I have seen such table for CAST(). Should > I ask Peter? I did it myself: thread is called "cast" in obsolete dev@tarantool mailing list (15 May 2019 12:44:54). There's even corresponding issue which was closed by Kirill for some reason: https://github.com/tarantool/tarantool/issues/4216 Now I am okay with these patches. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-04-10 10:39 ` Mergen Imeev 2020-04-10 10:43 ` Mergen Imeev @ 2020-04-10 12:46 ` Nikita Pettik 2020-04-10 13:05 ` Imeev Mergen 1 sibling, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-04-10 12:46 UTC (permalink / raw) To: Mergen Imeev; +Cc: tarantool-patches On 10 Apr 13:39, Mergen Imeev wrote: > 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? What about this? > > > 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) ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER 2020-04-10 12:46 ` Nikita Pettik @ 2020-04-10 13:05 ` Imeev Mergen 0 siblings, 0 replies; 18+ messages in thread From: Imeev Mergen @ 2020-04-10 13:05 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches On 4/10/20 3:46 PM, Nikita Pettik wrote: > On 10 Apr 13:39, Mergen Imeev wrote: >> 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? > What about this? Answered in follow-up letter: https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015650.html > >>>> 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) ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER 2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma @ 2020-03-27 11:33 ` imeevma 2020-03-27 16:54 ` Nikita Pettik 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma 2020-04-16 0:03 ` [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " Nikita Pettik 3 siblings, 1 reply; 18+ messages in thread From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw) To: korablev, tsafin, tarantool-patches Prior to this patch, STRING, which contains the DOUBLE value, could be implicitly 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. Example: box.execute("CREATE TABLE t(i INT PRIMARY KEY);") Before patch: box.execute("INSERT INTO t VALUES ('111.1');") box.execute("SELECT * FROM t;") Result: 111 After patch: box.execute("INSERT INTO t VALUES ('1.1');") Result: 'Type mismatch: can not convert 1.1 to integer' box.execute("INSERT INTO t VALUES ('1.0');") Result: 'Type mismatch: can not convert 1.0 to integer' box.execute("INSERT INTO t VALUES ('1.');") Result: 'Type mismatch: can not convert 1. to integer' Part of #4766 @TarantoolBot document Title: disallow cast from STRING contais DOUBLE to INTEGER After the last two patches, explicit and implicit casting from the string containing DOUBLE to INTEGER directly will be prohibited. The user must use the explicit cast to DOUBLE before the explicit or implicit cast to INTEGER. The reason for this is that before these patches, such STRINGs were implicitly cast to DOUBLE, and then this DOUBLE was implicitly or explicitly cast to INTEGER. Because of this, the result of such a cast may differ from what the user expects, and the user may not know why. Example for implicit cast: box.execute("CREATE TABLE t(i INT PRIMARY KEY);") -- Does not work anymore: box.execute("INSERT INTO t VALUES ('1.1');") -- Right way: box.execute("INSERT INTO t VALUES (CAST('1.1' AS DOUBLE));") Example for explicit cast: -- Does not work anymore: box.execute("SELECT CAST('1.1' AS INTEGER);") -- Right way: box.execute("SELECT CAST(CAST('1.1' AS DOUBLE) AS INTEGER);") --- src/box/sql/vdbe.c | 11 ++++- src/box/sql/vdbeInt.h | 1 - src/box/sql/vdbemem.c | 29 ------------ test/sql-tap/e_select1.test.lua | 2 +- .../gh-4766-wrong-cast-from-blob-to-int.test.lua | 55 +++++++++++++++++++++- test/sql-tap/intpkey.test.lua | 2 +- test/sql-tap/join.test.lua | 4 +- test/sql-tap/subquery.test.lua | 6 +-- test/sql-tap/tkt-9a8b09f8e6.test.lua | 4 +- 9 files changed, 72 insertions(+), 42 deletions(-) diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index e8a029a..6c0e5bd 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -319,6 +319,8 @@ mem_apply_type(struct Mem *record, enum field_type type) switch (type) { case FIELD_TYPE_INTEGER: case FIELD_TYPE_UNSIGNED: + if ((record->flags & (MEM_Bool | MEM_Blob)) != 0) + return -1; if ((record->flags & MEM_UInt) == MEM_UInt) return 0; if ((record->flags & MEM_Real) == MEM_Real) { @@ -331,8 +333,13 @@ mem_apply_type(struct Mem *record, enum field_type type) mem_set_u64(record, u); return 0; } - if (sqlVdbeMemIntegerify(record) != 0) - return -1; + if ((record->flags & MEM_Str) != 0) { + bool is_neg; + int64_t i; + if (sql_atoi64(record->z, &i, &is_neg, record->n) != 0) + return -1; + mem_set_int(record, i, is_neg); + } if ((record->flags & MEM_Int) == MEM_Int) { if (type == FIELD_TYPE_UNSIGNED) return -1; diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 38305ce..2c50b67 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -525,7 +525,6 @@ int sqlVdbeMemMakeWriteable(Mem *); int sqlVdbeMemStringify(Mem *); int sqlVdbeIntValue(Mem *, int64_t *, bool *is_neg); -int sqlVdbeMemIntegerify(struct Mem *pMem); int sqlVdbeRealValue(Mem *, double *); int diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index de1d9c3..3bc7303 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -551,35 +551,6 @@ mem_apply_integer_type(Mem *pMem) } /* - * Convert pMem to type integer. Invalidate any prior representations. - */ -int -sqlVdbeMemIntegerify(struct Mem *pMem) -{ - assert(EIGHT_BYTE_ALIGNMENT(pMem)); - - int64_t i; - bool is_neg; - if (sqlVdbeIntValue(pMem, &i, &is_neg) == 0) { - mem_set_int(pMem, i, is_neg); - return 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; -} - -/* * Convert pMem so that it is of type MEM_Real. * Invalidate any prior representations. */ diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua index c1818dd..1d3b964 100755 --- a/test/sql-tap/e_select1.test.lua +++ b/test/sql-tap/e_select1.test.lua @@ -2195,7 +2195,7 @@ test:do_select_tests( {"1", "SELECT b FROM f1 ORDER BY a LIMIT 0 ", {}}, {"2", "SELECT b FROM f1 ORDER BY a DESC LIMIT 4 ", {"z", "y", "x", "w"}}, {"3", "SELECT b FROM f1 ORDER BY a DESC LIMIT 8 ", {"z", "y", "x", "w", "v", "u", "t", "s"}}, - {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12.0' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}}, + {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}}, }) -- EVIDENCE-OF: R-54935-19057 Or, if the SELECT statement would return 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 index 0865c4e..559e33d 100755 --- 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 @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(6) +test:plan(12) -- -- Make sure that STRING or BLOB that contains DOUBLE value cannot @@ -54,4 +54,57 @@ test:do_catchsql_test( 1, "Type mismatch: can not convert varbinary to integer" }) +-- +-- Make sure that STRING or BLOB that contains DOUBLE value cannot +-- be implicitly cast to INTEGER. +-- +test:do_catchsql_test( + "gh-4766-7", + [[ + CREATE TABLE t(i INT PRIMARY KEY); + INSERT INTO t VALUES ('1.1'); + ]], { + 1, "Type mismatch: can not convert 1.1 to integer" + }) + +test:do_catchsql_test( + "gh-4766-8", + [[ + INSERT INTO t VALUES (x'312e31'); + ]], { + 1, "Type mismatch: can not convert varbinary to integer" + }) + +test:do_catchsql_test( + "gh-4766-9", + [[ + INSERT INTO t VALUES ('2.0'); + ]], { + 1, "Type mismatch: can not convert 2.0 to integer" + }) + +test:do_catchsql_test( + "gh-4766-10", + [[ + INSERT INTO t VALUES (x'322e30'); + ]], { + 1, "Type mismatch: can not convert varbinary to integer" + }) + +test:do_catchsql_test( + "gh-4766-11", + [[ + INSERT INTO t VALUES ('2.'); + ]], { + 1, "Type mismatch: can not convert 2. to integer" + }) + +test:do_catchsql_test( + "gh-4766-12", + [[ + INSERT INTO t VALUES (x'322e'); + ]], { + 1, "Type mismatch: can not convert varbinary to integer" + }) + test:finish_test() diff --git a/test/sql-tap/intpkey.test.lua b/test/sql-tap/intpkey.test.lua index bec2670..b6b1866 100755 --- a/test/sql-tap/intpkey.test.lua +++ b/test/sql-tap/intpkey.test.lua @@ -788,7 +788,7 @@ test:do_execsql_test( test:do_execsql_test( "intpkey-13.2", [[ - INSERT INTO t1 VALUES('1.0',2,3); + INSERT INTO t1 VALUES('1',2,3); SELECT * FROM t1 WHERE a=1; ]], { -- <intpkey-13.2> diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua index 4f014e0..840b780 100755 --- a/test/sql-tap/join.test.lua +++ b/test/sql-tap/join.test.lua @@ -1017,7 +1017,7 @@ test:do_test( return test:execsql [[ CREATE TABLE t1(a TEXT primary key, b TEXT); CREATE TABLE t2(b INTEGER primary key, a TEXT); - INSERT INTO t1 VALUES('one', '1.0'); + INSERT INTO t1 VALUES('one', '1'); INSERT INTO t1 VALUES('two', '2'); INSERT INTO t2 VALUES(1, 'one'); INSERT INTO t2 VALUES(2, 'two'); @@ -1034,7 +1034,7 @@ test:do_execsql_test( SELECT * FROM t1 NATURAL JOIN t2 ]], { -- <join-11.9> - "one", "1.0", "two", "2" + "one", "1", "two", "2" -- </join-11.9> }) diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua index 6bedf58..15c4c82 100755 --- a/test/sql-tap/subquery.test.lua +++ b/test/sql-tap/subquery.test.lua @@ -342,7 +342,7 @@ test:do_execsql_test( INSERT INTO t3 VALUES(10); CREATE TABLE t4(x TEXT PRIMARY KEY); - INSERT INTO t4 VALUES('10.0'); + INSERT INTO t4 VALUES('10'); ]], { -- <subquery-2.5.1> @@ -363,7 +363,7 @@ test:do_test( ]] end, { -- <subquery-2.5.2> - "10.0" + "10" -- </subquery-2.5.2> }) @@ -378,7 +378,7 @@ test:do_test( ]] end, { -- <subquery-2.5.3.1> - "10.0" + "10" -- </subquery-2.5.3.1> }) diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua index db0881c..cb5348a 100755 --- a/test/sql-tap/tkt-9a8b09f8e6.test.lua +++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua @@ -196,7 +196,7 @@ test:do_execsql_test( test:do_execsql_test( 3.4, [[ - SELECT x FROM t2 WHERE x IN ('1.0'); + SELECT x FROM t2 WHERE x IN ('1'); ]], { -- <3.4> 1 @@ -236,7 +236,7 @@ test:do_execsql_test( test:do_execsql_test( 3.8, [[ - SELECT x FROM t2 WHERE '1.0' IN (x); + SELECT x FROM t2 WHERE '1' IN (x); ]], { -- <3.8> 1 -- 2.7.4 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma @ 2020-03-27 16:54 ` Nikita Pettik 2020-04-10 10:41 ` Mergen Imeev 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-03-27 16:54 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches On 27 Mar 14:33, imeevma@tarantool.org wrote: > Prior to this patch, STRING, which contains the DOUBLE value, > could be implicitly 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. > > Example: > box.execute("CREATE TABLE t(i INT PRIMARY KEY);") > > Before patch: > box.execute("INSERT INTO t VALUES ('111.1');") > box.execute("SELECT * FROM t;") > Result: 111 > > After patch: > box.execute("INSERT INTO t VALUES ('1.1');") > Result: 'Type mismatch: can not convert 1.1 to integer' > > box.execute("INSERT INTO t VALUES ('1.0');") > Result: 'Type mismatch: can not convert 1.0 to integer' > > box.execute("INSERT INTO t VALUES ('1.');") > Result: 'Type mismatch: can not convert 1. to integer' Is comparison predicat affected? > Part of #4766 > > @TarantoolBot document > Title: disallow cast from STRING contais DOUBLE to INTEGER -> contains > 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 > index 0865c4e..559e33d 100755 > --- 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 Same nit concerning test file as in previous patch: personally I'd not introduce it. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER 2020-03-27 16:54 ` Nikita Pettik @ 2020-04-10 10:41 ` Mergen Imeev 2020-04-10 12:57 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Mergen Imeev @ 2020-04-10 10:41 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches On Fri, Mar 27, 2020 at 04:54:17PM +0000, Nikita Pettik wrote: > On 27 Mar 14:33, imeevma@tarantool.org wrote: > > Prior to this patch, STRING, which contains the DOUBLE value, > > could be implicitly 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. > > > > Example: > > box.execute("CREATE TABLE t(i INT PRIMARY KEY);") > > > > Before patch: > > box.execute("INSERT INTO t VALUES ('111.1');") > > box.execute("SELECT * FROM t;") > > Result: 111 > > > > After patch: > > box.execute("INSERT INTO t VALUES ('1.1');") > > Result: 'Type mismatch: can not convert 1.1 to integer' > > > > box.execute("INSERT INTO t VALUES ('1.0');") > > Result: 'Type mismatch: can not convert 1.0 to integer' > > > > box.execute("INSERT INTO t VALUES ('1.');") > > Result: 'Type mismatch: can not convert 1. to integer' > > Is comparison predicat affected? > No, since all implicit casts in case of comparison executed inside of comparisons opcodes. > > Part of #4766 > > > > @TarantoolBot document > > Title: disallow cast from STRING contais DOUBLE to INTEGER > > -> contains > Fixed. > > 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 > > index 0865c4e..559e33d 100755 > > --- 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 > > Same nit concerning test file as in previous patch: personally I'd > not introduce it. > Removed. New patch: From 18aed8fc1b1c566a90888c2835e4210ef3386009 Mon Sep 17 00:00:00 2001 From: Mergen Imeev <imeevma@gmail.com> Date: Thu, 26 Mar 2020 15:30:02 +0300 Subject: [PATCH] sql: fix implicit cast from STRING to INTEGER Prior to this patch, STRING, which contains the DOUBLE value, could be implicitly 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. Example: box.execute("CREATE TABLE t(i INT PRIMARY KEY);") Before patch: box.execute("INSERT INTO t VALUES ('111.1');") box.execute("SELECT * FROM t;") Result: 111 After patch: box.execute("INSERT INTO t VALUES ('1.1');") Result: 'Type mismatch: can not convert 1.1 to integer' box.execute("INSERT INTO t VALUES ('1.0');") Result: 'Type mismatch: can not convert 1.0 to integer' box.execute("INSERT INTO t VALUES ('1.');") Result: 'Type mismatch: can not convert 1. to integer' @TarantoolBot document Title: disallow cast from STRING contains DOUBLE to INTEGER After the last two patches, explicit and implicit casting from the string containing DOUBLE to INTEGER directly will be prohibited. The user must use the explicit cast to DOUBLE before the explicit or implicit cast to INTEGER. The reason for this is that before these patches, such STRINGs were implicitly cast to DOUBLE, and then this DOUBLE was implicitly or explicitly cast to INTEGER. Because of this, the result of such a cast may differ from what the user expects, and the user may not know why. Example for implicit cast: box.execute("CREATE TABLE t(i INT PRIMARY KEY);") -- Does not work anymore: box.execute("INSERT INTO t VALUES ('1.1');") -- Right way: box.execute("INSERT INTO t VALUES (CAST('1.1' AS DOUBLE));") Example for explicit cast: -- Does not work anymore: box.execute("SELECT CAST('1.1' AS INTEGER);") -- Right way: box.execute("SELECT CAST(CAST('1.1' AS DOUBLE) AS INTEGER);") diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index e8a029a..6c0e5bd 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -319,6 +319,8 @@ mem_apply_type(struct Mem *record, enum field_type type) switch (type) { case FIELD_TYPE_INTEGER: case FIELD_TYPE_UNSIGNED: + if ((record->flags & (MEM_Bool | MEM_Blob)) != 0) + return -1; if ((record->flags & MEM_UInt) == MEM_UInt) return 0; if ((record->flags & MEM_Real) == MEM_Real) { @@ -331,8 +333,13 @@ mem_apply_type(struct Mem *record, enum field_type type) mem_set_u64(record, u); return 0; } - if (sqlVdbeMemIntegerify(record) != 0) - return -1; + if ((record->flags & MEM_Str) != 0) { + bool is_neg; + int64_t i; + if (sql_atoi64(record->z, &i, &is_neg, record->n) != 0) + return -1; + mem_set_int(record, i, is_neg); + } if ((record->flags & MEM_Int) == MEM_Int) { if (type == FIELD_TYPE_UNSIGNED) return -1; diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h index 38305ce..2c50b67 100644 --- a/src/box/sql/vdbeInt.h +++ b/src/box/sql/vdbeInt.h @@ -525,7 +525,6 @@ int sqlVdbeMemMakeWriteable(Mem *); int sqlVdbeMemStringify(Mem *); int sqlVdbeIntValue(Mem *, int64_t *, bool *is_neg); -int sqlVdbeMemIntegerify(struct Mem *pMem); int sqlVdbeRealValue(Mem *, double *); int diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index de1d9c3..3bc7303 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -551,35 +551,6 @@ mem_apply_integer_type(Mem *pMem) } /* - * Convert pMem to type integer. Invalidate any prior representations. - */ -int -sqlVdbeMemIntegerify(struct Mem *pMem) -{ - assert(EIGHT_BYTE_ALIGNMENT(pMem)); - - int64_t i; - bool is_neg; - if (sqlVdbeIntValue(pMem, &i, &is_neg) == 0) { - mem_set_int(pMem, i, is_neg); - return 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; -} - -/* * Convert pMem so that it is of type MEM_Real. * Invalidate any prior representations. */ diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua index c1818dd..1d3b964 100755 --- a/test/sql-tap/e_select1.test.lua +++ b/test/sql-tap/e_select1.test.lua @@ -2195,7 +2195,7 @@ test:do_select_tests( {"1", "SELECT b FROM f1 ORDER BY a LIMIT 0 ", {}}, {"2", "SELECT b FROM f1 ORDER BY a DESC LIMIT 4 ", {"z", "y", "x", "w"}}, {"3", "SELECT b FROM f1 ORDER BY a DESC LIMIT 8 ", {"z", "y", "x", "w", "v", "u", "t", "s"}}, - {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12.0' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}}, + {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}}, }) -- EVIDENCE-OF: R-54935-19057 Or, if the SELECT statement would return diff --git a/test/sql-tap/intpkey.test.lua b/test/sql-tap/intpkey.test.lua index bec2670..b6b1866 100755 --- a/test/sql-tap/intpkey.test.lua +++ b/test/sql-tap/intpkey.test.lua @@ -788,7 +788,7 @@ test:do_execsql_test( test:do_execsql_test( "intpkey-13.2", [[ - INSERT INTO t1 VALUES('1.0',2,3); + INSERT INTO t1 VALUES('1',2,3); SELECT * FROM t1 WHERE a=1; ]], { -- <intpkey-13.2> diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua index 4f014e0..840b780 100755 --- a/test/sql-tap/join.test.lua +++ b/test/sql-tap/join.test.lua @@ -1017,7 +1017,7 @@ test:do_test( return test:execsql [[ CREATE TABLE t1(a TEXT primary key, b TEXT); CREATE TABLE t2(b INTEGER primary key, a TEXT); - INSERT INTO t1 VALUES('one', '1.0'); + INSERT INTO t1 VALUES('one', '1'); INSERT INTO t1 VALUES('two', '2'); INSERT INTO t2 VALUES(1, 'one'); INSERT INTO t2 VALUES(2, 'two'); @@ -1034,7 +1034,7 @@ test:do_execsql_test( SELECT * FROM t1 NATURAL JOIN t2 ]], { -- <join-11.9> - "one", "1.0", "two", "2" + "one", "1", "two", "2" -- </join-11.9> }) diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua index 6bedf58..15c4c82 100755 --- a/test/sql-tap/subquery.test.lua +++ b/test/sql-tap/subquery.test.lua @@ -342,7 +342,7 @@ test:do_execsql_test( INSERT INTO t3 VALUES(10); CREATE TABLE t4(x TEXT PRIMARY KEY); - INSERT INTO t4 VALUES('10.0'); + INSERT INTO t4 VALUES('10'); ]], { -- <subquery-2.5.1> @@ -363,7 +363,7 @@ test:do_test( ]] end, { -- <subquery-2.5.2> - "10.0" + "10" -- </subquery-2.5.2> }) @@ -378,7 +378,7 @@ test:do_test( ]] end, { -- <subquery-2.5.3.1> - "10.0" + "10" -- </subquery-2.5.3.1> }) diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua index db0881c..cb5348a 100755 --- a/test/sql-tap/tkt-9a8b09f8e6.test.lua +++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua @@ -196,7 +196,7 @@ test:do_execsql_test( test:do_execsql_test( 3.4, [[ - SELECT x FROM t2 WHERE x IN ('1.0'); + SELECT x FROM t2 WHERE x IN ('1'); ]], { -- <3.4> 1 @@ -236,7 +236,7 @@ test:do_execsql_test( test:do_execsql_test( 3.8, [[ - SELECT x FROM t2 WHERE '1.0' IN (x); + SELECT x FROM t2 WHERE '1' IN (x); ]], { -- <3.8> 1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER 2020-04-10 10:41 ` Mergen Imeev @ 2020-04-10 12:57 ` Nikita Pettik 2020-04-10 18:09 ` Mergen Imeev 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2020-04-10 12:57 UTC (permalink / raw) To: Mergen Imeev; +Cc: tarantool-patches On 10 Apr 13:41, Mergen Imeev wrote: > On Fri, Mar 27, 2020 at 04:54:17PM +0000, Nikita Pettik wrote: > > On 27 Mar 14:33, imeevma@tarantool.org wrote: > > > Prior to this patch, STRING, which contains the DOUBLE value, > > > could be implicitly 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. > > > > > > Example: > > > box.execute("CREATE TABLE t(i INT PRIMARY KEY);") > > > > > > Before patch: > > > box.execute("INSERT INTO t VALUES ('111.1');") > > > box.execute("SELECT * FROM t;") > > > Result: 111 > > > > > > After patch: > > > box.execute("INSERT INTO t VALUES ('1.1');") > > > Result: 'Type mismatch: can not convert 1.1 to integer' > > > > > > box.execute("INSERT INTO t VALUES ('1.0');") > > > Result: 'Type mismatch: can not convert 1.0 to integer' > > > > > > box.execute("INSERT INTO t VALUES ('1.');") > > > Result: 'Type mismatch: can not convert 1. to integer' > > > > Is comparison predicat affected? > > > No, since all implicit casts in case of comparison executed > inside of comparisons opcodes. You should have mentioned this fact in commit message. LGTM otherwise. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER 2020-04-10 12:57 ` Nikita Pettik @ 2020-04-10 18:09 ` Mergen Imeev 0 siblings, 0 replies; 18+ messages in thread From: Mergen Imeev @ 2020-04-10 18:09 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches Hi! Thank you for the review. I extended commit-message. You can see it below. On Fri, Apr 10, 2020 at 12:57:57PM +0000, Nikita Pettik wrote: > On 10 Apr 13:41, Mergen Imeev wrote: > > On Fri, Mar 27, 2020 at 04:54:17PM +0000, Nikita Pettik wrote: > > > On 27 Mar 14:33, imeevma@tarantool.org wrote: > > > > Prior to this patch, STRING, which contains the DOUBLE value, > > > > could be implicitly 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. > > > > > > > > Example: > > > > box.execute("CREATE TABLE t(i INT PRIMARY KEY);") > > > > > > > > Before patch: > > > > box.execute("INSERT INTO t VALUES ('111.1');") > > > > box.execute("SELECT * FROM t;") > > > > Result: 111 > > > > > > > > After patch: > > > > box.execute("INSERT INTO t VALUES ('1.1');") > > > > Result: 'Type mismatch: can not convert 1.1 to integer' > > > > > > > > box.execute("INSERT INTO t VALUES ('1.0');") > > > > Result: 'Type mismatch: can not convert 1.0 to integer' > > > > > > > > box.execute("INSERT INTO t VALUES ('1.');") > > > > Result: 'Type mismatch: can not convert 1. to integer' > > > > > > Is comparison predicat affected? > > > > > No, since all implicit casts in case of comparison executed > > inside of comparisons opcodes. > > You should have mentioned this fact in commit message. > Fixed: sql: fix implicit cast from STRING to INTEGER Prior to this patch, STRING, which contains the DOUBLE value, could be implicitly 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. It is worth noting that these changes will not affect the comparison, since the implicit cast in this case has different mechanics. Example: box.execute("CREATE TABLE t(i INT PRIMARY KEY);") Before patch: box.execute("INSERT INTO t VALUES ('111.1');") box.execute("SELECT * FROM t;") Result: 111 After patch: box.execute("INSERT INTO t VALUES ('1.1');") Result: 'Type mismatch: can not convert 1.1 to integer' box.execute("INSERT INTO t VALUES ('1.0');") Result: 'Type mismatch: can not convert 1.0 to integer' box.execute("INSERT INTO t VALUES ('1.');") Result: 'Type mismatch: can not convert 1. to integer' @TarantoolBot document Title: disallow cast from STRING contains DOUBLE to INTEGER After the last two patches, explicit and implicit casting from the string containing DOUBLE to INTEGER directly will be prohibited. The user must use the explicit cast to DOUBLE before the explicit or implicit cast to INTEGER. The reason for this is that before these patches, such STRINGs were implicitly cast to DOUBLE, and then this DOUBLE was implicitly or explicitly cast to INTEGER. Because of this, the result of such a cast may differ from what the user expects, and the user may not know why. It is worth noting that these changes will not affect the comparison, since the implicit cast in this case has different mechanics. Example for implicit cast: box.execute("CREATE TABLE t(i INT PRIMARY KEY);") -- Does not work anymore: box.execute("INSERT INTO t VALUES ('1.1');") -- Right way: box.execute("INSERT INTO t VALUES (CAST('1.1' AS DOUBLE));") Example for explicit cast: -- Does not work anymore: box.execute("SELECT CAST('1.1' AS INTEGER);") -- Right way: box.execute("SELECT CAST(CAST('1.1' AS DOUBLE) AS INTEGER);") > LGTM otherwise. > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast to INTEGER 2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma @ 2020-03-27 11:33 ` imeevma 2020-03-27 16:54 ` Nikita Pettik 2020-04-16 0:03 ` [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " Nikita Pettik 3 siblings, 1 reply; 18+ messages in thread From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw) To: korablev, tsafin, tarantool-patches Prior to this patch, due to the absence of the '\0' character at the end of the BLOB, it was possible to get an error or incorrect result when using CAST() from BLOB to INTEGER or UNSIGNED. This has now been fixed, but the maximum length of a BLOB that could be cast to INTEGER or UNSIGNED was limited to 12287 bytes. Examples of wrong CAST() from BLOB to INTEGER: CREATE TABLE t (i INT PRIMARY KEY, a VARBINARY, b INT, c INT); INSERT INTO t VALUES (1, X'33', 0x33, 0x00), (2, X'34', 0x41, 0); Example of wrong result: SELECT CAST(a AS INTEGER) FROM t WHERE i = 1; Result: 33 Example of error during CAST(): SELECT CAST(a AS INTEGER) FROM t WHERE i = 2; Result: 'Type mismatch: can not convert varbinary to integer' Closes #4766 --- src/box/sql/util.c | 17 ++++++--- .../gh-4766-wrong-cast-from-blob-to-int.test.lua | 42 +++++++++++++++++++++- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/box/sql/util.c b/src/box/sql/util.c index f908e9c..c556b98 100644 --- a/src/box/sql/util.c +++ b/src/box/sql/util.c @@ -467,14 +467,21 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length) if (*z == '-') *is_neg = true; + /* + * BLOB data may not end with '\0'. Because of this, the + * strtoll() and strtoull() functions may return an + * incorrect result. To fix this, let's copy the value for + * decoding into static memory and add '\0' to it. + */ + if (length > SMALL_STATIC_SIZE - 1) + return -1; + const char *str_value = tt_cstr(z, length); char *end = NULL; errno = 0; - if (*z == '-') { - *is_neg = true; - *val = strtoll(z, &end, 10); + if (*is_neg) { + *val = strtoll(str_value, &end, 10); } else { - *is_neg = false; - uint64_t u_val = strtoull(z, &end, 10); + uint64_t u_val = strtoull(str_value, &end, 10); *val = u_val; } /* Overflow and underflow errors. */ 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 index 559e33d..37aae87 100755 --- 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 @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(12) +test:plan(15) -- -- Make sure that STRING or BLOB that contains DOUBLE value cannot @@ -107,4 +107,44 @@ test:do_catchsql_test( 1, "Type mismatch: can not convert varbinary to integer" }) +-- +-- Make sure that a blob as part of a tuple can be cast to NUMBER, +-- INTEGER and UNSIGNED. Prior to this patch, an error could +-- appear due to the absence of '\0' at the end of the BLOB. +-- +test:do_execsql_test( + "gh-4766-13", + [[ + CREATE TABLE t1 (a VARBINARY PRIMARY KEY); + INSERT INTO t1 VALUES (X'33'), (X'372020202020'); + SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t1; + ]], { + '3', 3, 3, 3, '7 ', 7, 7, 7 + }) + +-- +-- Make sure that BLOB longer than 12287 bytes cannot be cast to +-- INTEGER. +-- +long_str = string.rep('0', 12284) +test:do_execsql_test( + "gh-4766-14", + "SELECT CAST('" .. long_str .. "123'" .. " AS INTEGER);", { + 123 + }) + + +test:do_catchsql_test( + "gh-4766-15", + "SELECT CAST('" .. long_str .. "1234'" .. " AS INTEGER);", { + 1, "Type mismatch: can not convert 000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "0000000000000000000000000000000000000000000000000000000000000000000" .. + "000000000000000000000000000000000000000000000" + }) + test:finish_test() -- 2.7.4 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast to INTEGER 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma @ 2020-03-27 16:54 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2020-03-27 16:54 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches On 27 Mar 14:33, imeevma@tarantool.org wrote: LGTM ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER 2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma ` (2 preceding siblings ...) 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma @ 2020-04-16 0:03 ` Nikita Pettik 3 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2020-04-16 0:03 UTC (permalink / raw) To: imeevma; +Cc: tarantool-patches On 27 Mar 14:33, imeevma@tarantool.org wrote: > This patch-set fixes CAST() from BLOB to INTEGER in case a BLOB > does not have '\0'. > > https://github.com/tarantool/tarantool/issues/4766 > https://github.com/tarantool/tarantool/tree/imeevma/gh-4766-fix-blob-size-for-cast > > @Changelog > - Explicit and implicit cast from string contains DOUBLE value to > INTEGER or UNSIGNED was disallowed. > - Maximum length of a BLOB that is allowed to be cast to INTEGER > or UNSIGNED was limited to 12287 bytes. > - Fixed wrong behaviour of CAST() from BLOB to INTEGER in case a > BLOB does not have '\0'. (gh-4766) Pushed to master, updated changelog, dropped branch. > Mergen Imeev (3): > sql: fix CAST() from STRING to INTEGER > sql: fix implicit cast from STRING to INTEGER > sql: add '\0' to the BLOB when it is cast to INTEGER > > src/box/sql/util.c | 17 ++- > src/box/sql/vdbe.c | 11 +- > src/box/sql/vdbeInt.h | 1 - > src/box/sql/vdbemem.c | 45 ++----- > test/sql-tap/e_select1.test.lua | 2 +- > .../gh-4766-wrong-cast-from-blob-to-int.test.lua | 150 +++++++++++++++++++++ > test/sql-tap/intpkey.test.lua | 2 +- > test/sql-tap/join.test.lua | 4 +- > test/sql-tap/subquery.test.lua | 6 +- > test/sql-tap/tkt-9a8b09f8e6.test.lua | 4 +- > test/sql/types.result | 23 ++-- > 11 files changed, 203 insertions(+), 62 deletions(-) > create mode 100755 test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua > > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-04-16 0:03 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma 2020-03-27 16:46 ` Nikita Pettik 2020-04-10 10:39 ` Mergen Imeev 2020-04-10 10:43 ` Mergen Imeev 2020-04-10 13:05 ` Nikita Pettik 2020-04-10 17:06 ` Imeev Mergen 2020-04-15 11:13 ` Nikita Pettik 2020-04-10 12:46 ` Nikita Pettik 2020-04-10 13:05 ` Imeev Mergen 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma 2020-03-27 16:54 ` Nikita Pettik 2020-04-10 10:41 ` Mergen Imeev 2020-04-10 12:57 ` Nikita Pettik 2020-04-10 18:09 ` Mergen Imeev 2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma 2020-03-27 16:54 ` Nikita Pettik 2020-04-16 0:03 ` [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " Nikita Pettik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox