From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Mergen Imeev <imeevma@tarantool.org>, tsafin@tarantool.org, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 5/6] sql: remove implicit cast from string for comparison Date: Mon, 1 Jun 2020 19:04:14 +0200 [thread overview] Message-ID: <379debee-67fc-f870-9e0b-032a5188d8a4@tarantool.org> (raw) In-Reply-To: <49b7078dd5f787e2dcde37554522afbe63f1d994.1590671266.git.imeevma@gmail.com> 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.
next prev parent reply other threads:[~2020-06-01 17:04 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-05-28 14:17 [Tarantool-patches] [PATCH 0/6] Remove implicit cast Mergen Imeev 2020-05-28 14:17 ` [Tarantool-patches] [PATCH 1/6] sql: remove implicit cast for assignment Mergen Imeev 2020-06-01 17:03 ` Vladislav Shpilevoy 2020-06-09 11:41 ` Mergen Imeev 2020-06-09 22:28 ` Vladislav Shpilevoy 2020-05-28 14:17 ` [Tarantool-patches] [PATCH 2/6] sql: remove mem_apply_type() from OP_MakeRecord Mergen Imeev 2020-06-01 17:03 ` Vladislav Shpilevoy 2020-06-09 11:43 ` Mergen Imeev 2020-05-28 14:17 ` [Tarantool-patches] [PATCH 3/6] sql: replace ApplyType by CheckType for IN operator Mergen Imeev 2020-05-28 14:17 ` [Tarantool-patches] [PATCH 4/6] sql: remove mem_apply_type() from OP_MustBeInt Mergen Imeev 2020-06-01 17:04 ` Vladislav Shpilevoy 2020-06-09 11:47 ` Mergen Imeev 2020-06-09 22:28 ` Vladislav Shpilevoy 2020-05-28 14:17 ` [Tarantool-patches] [PATCH 5/6] sql: remove implicit cast from string for comparison Mergen Imeev 2020-06-01 17:04 ` Vladislav Shpilevoy [this message] 2020-06-09 11:51 ` Mergen Imeev 2020-06-09 22:29 ` Vladislav Shpilevoy 2020-05-28 14:17 ` [Tarantool-patches] [PATCH 6/6] sql: remove OP_ApplyType Mergen Imeev 2020-06-09 22:29 ` Vladislav Shpilevoy 2020-06-01 17:03 ` [Tarantool-patches] [PATCH 0/6] Remove implicit cast Vladislav Shpilevoy 2020-06-09 11:25 ` Mergen Imeev
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=379debee-67fc-f870-9e0b-032a5188d8a4@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=tsafin@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 5/6] sql: remove implicit cast from string for comparison' \ /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