From: Mergen Imeev <imeevma@tarantool.org> To: Nikita Pettik <korablev@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER Date: Fri, 10 Apr 2020 13:43:52 +0300 [thread overview] Message-ID: <20200410104352.GA20241@tarantool.org> (raw) In-Reply-To: <20200410103945.GA20019@tarantool.org> 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);") > ---
next prev parent reply other threads:[~2020-04-10 10:43 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " 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 [this message] 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
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200410104352.GA20241@tarantool.org \ --to=imeevma@tarantool.org \ --cc=korablev@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox