[Tarantool-patches] [PATCH 5/6] sql: remove implicit cast from string for comparison
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jun 1 20:04:14 MSK 2020
Thanks for the patch!
See 7 comments below.
On 28/05/2020 16:17, Mergen Imeev wrote:
> This patch removes implicit cast from strings to numbers for
> comparison.
>
> Closes #4230
>
> @TarantoolBot document
> Title: remove implicit cast between strings and numbers
>
> This patch-set removes implicit cast from string to number and
> from number to string.
>
> Example:
>
> For comparison:
>
> tarantool> box.execute([[SELECT '1' > 0;]])
> ---
> - null
> - 'Type mismatch: can not convert 1 to numeric'
> ...
>
> tarantool> box.execute([[SELECT "id" FROM "_space" WHERE '1' > "id";]])
> ---
> - null
> - 'Type mismatch: can not convert text to unsigned'
> ...
>
> For assignment:
>
> tarantool> box.execute([[CREATE TABLE t1(i INT PRIMARY KEY);]])
> tarantool> box.execute([[INSERT INTO t1 VALUES ('1');]])
> ---
> - null
> - 'Type mismatch: can not convert text to integer'
> ...
>
> tarantool> box.execute([[CREATE TABLE t2(t text PRIMARY KEY);]])
> tarantool> box.execute([[INSERT INTO t2 VALUES (1);]])
> ---
> - null
> - 'Type mismatch: can not convert unsigned to string'
> ...
> ---
> src/box/sql/vdbe.c | 105 ++++----
> src/box/sql/wherecode.c | 203 +--------------
> .../gh-4230-del-impl-cast-str-to-num.test.lua | 78 ++++++
> test/sql-tap/identifier_case.test.lua | 6 +-
> test/sql-tap/in1.test.lua | 4 +-
> test/sql-tap/in4.test.lua | 3 +-
> test/sql-tap/insert3.test.lua | 2 +-
> test/sql-tap/intpkey.test.lua | 24 +-
> test/sql-tap/join.test.lua | 7 +-
> test/sql-tap/misc1.test.lua | 32 +--
> test/sql-tap/select1.test.lua | 4 +-
> test/sql-tap/select7.test.lua | 2 +-
> test/sql-tap/subquery.test.lua | 4 +-
> test/sql-tap/tkt-9a8b09f8e6.test.lua | 84 ++++---
> test/sql-tap/tkt-f973c7ac31.test.lua | 32 +--
> test/sql-tap/tkt-fc7bd6358f.test.lua | 6 +-
> test/sql-tap/tkt3493.test.lua | 40 ++-
> test/sql-tap/transitive1.test.lua | 12 +-
> test/sql-tap/where2.test.lua | 183 +-------------
> test/sql-tap/where5.test.lua | 12 +-
> test/sql-tap/whereB.test.lua | 238 ++++++++++--------
> test/sql/types.result | 12 +-
> test/sql/types.test.lua | 1 -
> 23 files changed, 418 insertions(+), 676 deletions(-)
> create mode 100755 test/sql-tap/gh-4230-del-impl-cast-str-to-num.test.lua
>
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 7add67ae7..021e09d1e 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2345,22 +2338,17 @@ case OP_Ge: { /* same as TK_GE, jump, in1, in3 */
> goto compare_op;
> }
> } else if (type == FIELD_TYPE_STRING) {
> - if ((flags1 & MEM_Str) == 0 &&
> - (flags1 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
> - testcase( pIn1->flags & MEM_Int);
> - testcase( pIn1->flags & MEM_Real);
> - sqlVdbeMemStringify(pIn1);
> - testcase( (flags1&MEM_Dyn) != (pIn1->flags&MEM_Dyn));
> - flags1 = (pIn1->flags & ~MEM_TypeMask) | (flags1 & MEM_TypeMask);
> - assert(pIn1!=pIn3);
> + if ((flags1 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
1. Maybe it would be better to check '(flags1 & MEM_Str) == 0'. Otherwise
every time when we will add a new type, we will need to add it here. The
same below.
> + diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> + mem_type_to_str(pIn3),
> + mem_type_to_str(pIn1));
> + goto abort_due_to_error;
> }
> - if ((flags3 & MEM_Str) == 0 &&
> - (flags3 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
> - testcase( pIn3->flags & MEM_Int);
> - testcase( pIn3->flags & MEM_Real);
> - sqlVdbeMemStringify(pIn3);
> - testcase( (flags3&MEM_Dyn) != (pIn3->flags&MEM_Dyn));
> - flags3 = (pIn3->flags & ~MEM_TypeMask) | (flags3 & MEM_TypeMask);
> + if ((flags3 & (MEM_Int | MEM_UInt | MEM_Real)) != 0) {
> + diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> + mem_type_to_str(pIn1),
> + mem_type_to_str(pIn3));
> + goto abort_due_to_error;
> }
> }
> assert(pOp->p4type==P4_COLLSEQ || pOp->p4.pColl==0);
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index 5bc27f134..1d7c76670 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -644,8 +578,7 @@ static int
> codeAllEqualityTerms(Parse * pParse, /* Parsing context */
> WhereLevel * pLevel, /* Which nested loop of the FROM we are coding */
> int bRev, /* Reverse the order of IN operators */
> - int nExtraReg, /* Number of extra registers to allocate */
> - enum field_type **res_type)
> + int nExtraReg) /* Number of extra registers to allocate */
> {
> u16 nEq; /* The number of == or IN constraints to code */
> u16 nSkip; /* Number of left-most columns to skip */
> @@ -669,9 +602,6 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */
> nReg = pLoop->nEq + nExtraReg;
> pParse->nMem += nReg;
>
> - enum field_type *type = sql_index_type_str(pParse->db, idx_def);
2. This function is now unused and can be deleted.
> - assert(type != NULL || pParse->db->mallocFailed);
> -
> if (nSkip) {
> int iIdxCur = pLevel->iIdxCur;
> sqlVdbeAddOp1(v, (bRev ? OP_Last : OP_Rewind), iIdxCur);> diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
> index 854ed774f..2a18b17be 100755
> --- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
> +++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
> @@ -83,23 +83,23 @@ test:do_execsql_test(
> -- </1.5>
> })
>
> -test:do_execsql_test(
> +test:do_catchsql_test(
> 2.1,
> [[
> SELECT x FROM t1 WHERE x IN (1);
> ]], {
> -- <2.1>
> - "1"
> + 1,"Type mismatch: can not convert 1 to numeric"
3. Can tests in this file be fixed to return the same results
as before?
> -- </2.1>
> })
>
> -test:do_execsql_test(
> +test:do_catchsql_test(
> 2.2,
> [[
> SELECT x FROM t1 WHERE x IN (1.0);
> ]], {
> -- <2.2>
> - "1"
> + 1,"Type mismatch: can not convert 1 to numeric"
> -- </2.2>
> })
>
> diff --git a/test/sql-tap/tkt-f973c7ac31.test.lua b/test/sql-tap/tkt-f973c7ac31.test.lua
> index 82bdb52f8..5239a7785 100755
> --- a/test/sql-tap/tkt-f973c7ac31.test.lua
> +++ b/test/sql-tap/tkt-f973c7ac31.test.lua
> @@ -36,12 +36,12 @@ local sqls = {
>
> for tn, sql in ipairs(sqls) do
> test:execsql(sql)
> - test:do_execsql_test(
> + test:do_catchsql_test(
> "tkt-f973c7ac3-1."..tn..".1",
> [[
> SELECT c1,c2 FROM t WHERE c1 = 5 AND c2>0 AND c2<='2' ORDER BY c2 DESC
> ]], {
> -
> + 1, "/Type mismatch: can not convert/"
4. The same for this file.
> })
>
> test:do_execsql_test(
> diff --git a/test/sql-tap/tkt3493.test.lua b/test/sql-tap/tkt3493.test.lua
> index de77e61e9..ec12a4492 100755
> --- a/test/sql-tap/tkt3493.test.lua
> +++ b/test/sql-tap/tkt3493.test.lua
> @@ -126,23 +126,13 @@ test:do_execsql_test(
> test:do_execsql_test(
> "tkt3493-2.2.1",
> [[
> - SELECT a=123 FROM t1 GROUP BY a
> + SELECT a='123' FROM t1 GROUP BY a
5. Trailing whitespace.
> ]], {
> -- <tkt3493-2.2.1>
> true
> -- </tkt3493-2.2.1>
> })
> @@ -166,7 +156,7 @@ test:do_execsql_test(
> test:do_execsql_test(
> "tkt3493-2.2.5",
> [[
> - SELECT count(*), +a=123 FROM t1
> + SELECT count(*), +a='123' FROM t1
6. Trailing whitespace here and in the test above.
> ]], {
> -- <tkt3493-2.2.5>
> 1, true
> diff --git a/test/sql-tap/whereB.test.lua b/test/sql-tap/whereB.test.lua
> index 970ff1dec..9e99ea41c 100755
> --- a/test/sql-tap/whereB.test.lua
> +++ b/test/sql-tap/whereB.test.lua
> @@ -36,49 +35,54 @@ test:do_execsql_test(
> CREATE TABLE t2(a INT primary key, b TEXT); -- affinity of t2.b is TEXT
> CREATE INDEX t2b ON t2(b);
> INSERT INTO t2 VALUES(2,'99');
> + ]]
> +)
>
> +test:do_catchsql_test(
> + "whereB-1.1",
7. The whole whereB-1 now fails because of type conversions banned.
Is this test still needed at all? I didn't check other whereB tests,
but seems like most of them also fail now. So either almost the entire
test file is not needed, or it should be fixed so as the test still
would work.
More information about the Tarantool-patches
mailing list