From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 58BCD2CC32 for ; Wed, 17 Oct 2018 17:45:30 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GFbpcsBYcNI5 for ; Wed, 17 Oct 2018 17:45:30 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6D2542BCF0 for ; Wed, 17 Oct 2018 17:45:29 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions References: <3ba8b43c-c6d5-ff34-1fd2-c302eae1a760@tarantool.org> <98FC4C78-08BF-4B69-8F79-9562F1285432@tarantool.org> From: Vladislav Shpilevoy Message-ID: <999eb301-5c83-f1d8-491d-87c52ab5815f@tarantool.org> Date: Thu, 18 Oct 2018 00:45:24 +0300 MIME-Version: 1.0 In-Reply-To: <98FC4C78-08BF-4B69-8F79-9562F1285432@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, "n.pettik" Thanks for the patch! See 10 comments, review fixes on the branch and here. Also, please, elaborate what is wrong with tests - they do not pass. On 12/10/2018 14:19, n.pettik wrote: > >>> From: Georgy Kirichenko >>> Most DBs (at least PostgreSQL, Oracle and DB2) allow to process >>> following queries: >>> CREATE TABLE t1 (id INT PRIMARY KEY); >>> INSERT INTO t1 VALUES (1.123), ('2'); >>> In this particular case, 1.123 should be simply truncated to 1, >>> and '2' - converted to literal number 2. >>> After passing real type to Tarantool (instead of ), example >>> above would fail without conversions. Thus, lets add implicit >>> conversions inside VDBE to make this example be legal. >>> However, still some types conversions must be prohibited. For instance, >>> can't be converted to integer or floating point numerical, >>> and vice versa. >> >> As I see in the tests that it looks weird now: >> >> I can insert into 'int' a 'float' value, but can not >> compare them: >> >> box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<2.001") >> --- >> -- error: Can't convert 2.001 to INTEGER >> ... >> >> Why? We should either forbid insertion and comparison, or >> allow both of them. > > Well, now I agree that it looks quite strange, but I can't tell you > why I did so. It took quite a long to fix that, but workaround turned > out the be trivial: > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3d2324867..827811cd1 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -335,6 +335,14 @@ mem_apply_affinity(struct Mem *record, enum affinity_type affinity) > case AFFINITY_INTEGER: > if ((record->flags & MEM_Int) == MEM_Int) > return 0; > + if ((record->flags & MEM_Real) == MEM_Real) { > + int64_t i = (int64_t) record->u.r; > + if (i == record->u.r) { > + record->u.i = i; > + MemSetTypeFlag(record, MEM_Int); > + } > + return 0; > + } > return sqlite3VdbeMemIntegerify(record, false); > case AFFINITY_REAL: > if ((record->flags & MEM_Real) == MEM_Real) > @@ -1918,6 +1926,13 @@ case OP_MustBeInt: { /* jump, in1 */ > pIn1 = &aMem[pOp->p1]; > if ((pIn1->flags & MEM_Int)==0) { > mem_apply_affinity(pIn1, AFFINITY_INTEGER); > + if ((pIn1->flags & MEM_Real) == MEM_Real) { > + int64_t i = (int64_t) pIn1->u.r; > + if (i == pIn1->u.r) { > + pIn1->u.i = i; > + MemSetTypeFlag(pIn1, MEM_Int); > + } > + } 1. Why? mem_apply_affinity does the same and is called one line above, it is not? > VdbeBranchTaken((pIn1->flags&MEM_Int)==0, 2); > if ((pIn1->flags & MEM_Int)==0) { > if (pOp->p2==0) { > @@ -3463,7 +3478,6 @@ case OP_SeekGT: { /* jump, in3 */ > reg_ipk = pOp->p5; > > if (reg_ipk > 0) { > - > /* The input value in P3 might be of any type: integer, real, string, > * blob, or NULL. But it needs to be an integer before we can do > * the seek, so convert it. > @@ -3473,9 +3487,18 @@ case OP_SeekGT: { /* jump, in3 */ > applyNumericAffinity(pIn3, 0); > } > int64_t i; > - if (sqlite3VdbeIntValue(pIn3, &i) != 0) { > + if ((pIn3->flags & MEM_Int) == MEM_Int) { > + i = pIn3->u.i; > + } else if ((pIn3->flags & MEM_Real) == MEM_Real) { > + if (pIn3->u.r > INT64_MAX) > + i = INT64_MAX; > + else if (pIn3->u.r < INT64_MIN) > + i = INT64_MIN; 2. In the explanation below you say 'without loses' but here they are, it is not? An example: I do search by SeekLT with something bigger than INT64_MAX and the index really has INT64_MAX. You will turn this query into SeekLT by INT64_MAX, so INT64_MAX will not be presented in the result, but should be. I guess, if I am right, you can fix it easy just changing OP_SeekLT to OP_SeekLE in such a case, and the same for OP_SeekGT/GE and INT64_MIN. > + else > + i = pIn3->u.r; > + } else { > *Explanation* > > Now OP_Affinity doesn’t ‘integrify’ float since it leads to losing > information concerning the fact that initial ‘real’ value was greater or less > than truncated ‘int’. Instead, it is done by explicit OP_MustBeInt conversion > OR when we are already in comparing routine and do it implicitly. > The only one case when OP_Affinity changes float to int is situation > when float can be casted to int without loses. > >>> + >>> + sqlite3VdbeError(p, format, sqlite3_value_text(pIn1)); >>> + goto abort_due_to_error; >>> + } >>> break; >>> } >>> #endif /* SQLITE_OMIT_CAST */ >>> @@ -2451,11 +2516,13 @@ case OP_IfNot: { /* jump, in1 */ >>> if (pIn1->flags & MEM_Null) { >>> c = pOp->p3; >>> } else { >>> -#ifdef SQLITE_OMIT_FLOATING_POINT >>> - c = sqlite3VdbeIntValue(pIn1)!=0; >>> -#else >>> - c = sqlite3VdbeRealValue(pIn1)!=0.0; >>> -#endif >>> + double v; >>> + if ((rc = sqlite3VdbeRealValue(pIn1, &v))) { >>> + sqlite3VdbeError(p, "Can't convert to numeric %s", >> >> 3. Why numeric? Maybe real? > > It doesn’t really matter. Currently they are the same. 3. I know, but when a user asks to cast to real or just operates on real, and gets an error message "Can't convert to numeric", it is strange. > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c > index 072a05066..f9527b650 100644 > --- a/src/box/sql/vdbemem.c > +++ b/src/box/sql/vdbemem.c > /* > * Convert pMem to type integer. Invalidate any prior representations. > */ > int > -sqlite3VdbeMemIntegerify(Mem * pMem) > +sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced) > { > assert(EIGHT_BYTE_ALIGNMENT(pMem)); > > - pMem->u.i = sqlite3VdbeIntValue(pMem); > + int64_t i; > + if (sqlite3VdbeIntValue(pMem, &i) == 0) { > + pMem->u.i = i; > + MemSetTypeFlag(pMem, MEM_Int); > + return SQLITE_OK; > + } else if ((pMem->flags & MEM_Real) != 0 && is_forced) { > + pMem->u.i = (int) pMem->u.r; > + MemSetTypeFlag(pMem, MEM_Int); > + return 0; 4. Lets do not mix SQLITE_OK/0 in one function. > + } > + > + double d; > + if (sqlite3VdbeRealValue(pMem, &d) || (int64_t) d != d) { > + return SQLITE_ERROR; > + } > + pMem->u.i = (int64_t) d; > MemSetTypeFlag(pMem, MEM_Int); > return SQLITE_OK; > } > @@ -583,46 +601,47 @@ sqlite3VdbeMemNumerify(Mem * pMem) > * affinity even if that results in loss of data. This routine is > * used (for example) to implement the SQL "cast()" operator. > */ > -void > +int > sqlite3VdbeMemCast(Mem * pMem, u8 aff) > { > if (pMem->flags & MEM_Null) > - return; > - switch (aff) { > - case AFFINITY_BLOB:{ /* Really a cast to BLOB */ > - if ((pMem->flags & MEM_Blob) == 0) { > - sqlite3ValueApplyAffinity(pMem, AFFINITY_TEXT); > - assert(pMem->flags & MEM_Str > - || pMem->db->mallocFailed); > - if (pMem->flags & MEM_Str) > - MemSetTypeFlag(pMem, MEM_Blob); > - } else { > - pMem->flags &= ~(MEM_TypeMask & ~MEM_Blob); > - } > - break; > - } > - case AFFINITY_NUMERIC:{ > - sqlite3VdbeMemNumerify(pMem); > - break; > - } > - case AFFINITY_INTEGER:{ > - sqlite3VdbeMemIntegerify(pMem); > - break; > - } > - case AFFINITY_REAL:{ > - sqlite3VdbeMemRealify(pMem); > - break; > + return SQLITE_OK; > + if (pMem->flags & MEM_Blob && aff == AFFINITY_INTEGER) > + return sql_atoi64(pMem->z, &pMem->u.i, pMem->n); > + if (pMem->flags & MEM_Blob && > + (aff == AFFINITY_REAL || aff == AFFINITY_NUMERIC)) { > + if (sql_atoi64(pMem->z, &pMem->u.i, pMem->n) == 0) { > + MemSetTypeFlag(pMem, MEM_Real); > + pMem->u.r = pMem->u.i; > + return 0; > } 5. I guess, you can move these two checks down to the switch into corresponding cases to avoid unnecessary checks. See my review fixes. And if you applied them, I would answer why not to fix sqlite3VdbeIntValue to allow cast BLOB to int as you do here out of sqlite3VdbeIntValue? I mean to delete this 'if' above: if (pMem->flags & MEM_Blob && aff == AFFINITY_INTEGER) and allow 'case AFFINITY_INTEGER:' do its work below. I did not move this case: if ((pMem->flags & MEM_Blob) != 0 && (aff == AFFINITY_REAL || aff == AFFINITY_NUMERIC)) { Please, do it by yourself. > diff --git a/test/sql-tap/boundary2.test.lua b/test/sql-tap/boundary2.test.lua > index 3eaef75dc..be4b8750d 100755 > --- a/test/sql-tap/boundary2.test.lua > +++ b/test/sql-tap/boundary2.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test = require("sqltester") > -test:plan(3021) > +test:plan(2965) > > --!./tcltestrunner.lua > -- 2008 December 11 > @@ -7462,6 +7462,7 @@ test:do_execsql_test( > "SELECT a FROM t1 WHERE r > 9.22337303685477580800e+18 ORDER BY a DESC", > {}) > > +if false then 6. I thought you have removed all these 'if false'. Please, do it. > test:do_execsql_test( > "boundary2-2.65.gt.3", > "SELECT a FROM t1 WHERE r > 9.22337303685477580800e+18 ORDER BY r", > diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua > index eb4f43a90..e5a2d7a5a 100755 > --- a/test/sql-tap/collation.test.lua > +++ b/test/sql-tap/collation.test.lua > @@ -250,8 +250,8 @@ local like_testcases = > test:do_catchsql_set_test(like_testcases, prefix) > > test:do_catchsql_test( > - "collation-2.5.0", > - 'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))', > - {1, "Collation 'FOO' does not exist"}) > + "collation-2.5.0", > + 'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))', > + {1, "Collation 'FOO' does not exist"}) 7. Nothing has changed. Stray diff. > > test:finish_test() > diff --git a/test/sql-tap/like3.test.lua b/test/sql-tap/like3.test.lua > index ea6824ba7..a683df2d6 100755 > --- a/test/sql-tap/like3.test.lua > +++ b/test/sql-tap/like3.test.lua > @@ -75,9 +75,9 @@ test:do_execsql_test( > CREATE INDEX t2ba ON t2(b,a); > SELECT a, b FROM t2 WHERE b GLOB 'ab*' ORDER BY +a; > ]], { > - -- > - 1, "abc", 4, "abc" > - -- > + -- > + 1, "abc", 4, "abc" > + -- 8. The same, and below. > }) > > test:do_execsql_test( > @@ -85,54 +85,55 @@ test:do_execsql_test( > [[ > SELECT a, b FROM t2 WHERE +b GLOB 'ab*' ORDER BY +a; > ]], { > - -- > - 1, "abc", 4, "abc" > - -- > - }) > + -- > + 1, "abc", 4, "abc" > + -- > + }) > > test:execsql([[ > CREATE TABLE t3(x TEXT PRIMARY KEY COLLATE "unicode_ci"); > INSERT INTO t3(x) VALUES('aaa'),('abc'),('abd'),('abe'),('acz'); > - INSERT INTO t3(x) SELECT CAST(x AS blob) FROM t3; > +-- INSERT INTO t3(x) SELECT CAST(x AS blob) FROM t3; 9. Delete or uncomment, please. > ]]) > > -- MUST_WORK #1476 collate nocase > diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua > index f917e5a51..6750cefef 100755 > --- a/test/sql-tap/numcast.test.lua > +++ b/test/sql-tap/numcast.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test = require("sqltester") > -test:plan(17) > +test:plan(9) > > --!./tcltestrunner.lua > -- 2013 March 20 > @@ -38,11 +38,8 @@ for _, enc in ipairs({"utf8"}) do > {"1", "12345.0", 12345.0, 12345}, > {"2", "12345.0e0", 12345.0, 12345}, > {"3", "-12345.0e0", -12345.0, -12345}, > - {"4", "-12345.25", -12345.25, -12345}, > +-- {"4", "-12345.25", -12345.25, -12345}, 10. -- > {"5", "-12345.0", -12345.0, -12345}, > - {"6", "'876xyz'", 876.0, 876}, > - {"7", "'456ķ89'", 456.0, 456}, > - {"8", "'Ġ 321.5'", 0.0, 0}, > } > for _, val in ipairs(data) do > local idx = val[1] Review fixes: ====================================================================== diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index 55bdeb4e3..5ad643f25 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -283,14 +283,10 @@ sqlite3VdbeMemStringify(Mem * pMem, u8 bForce) int fg = pMem->flags; const int nByte = 32; - if (fg & MEM_Null) - return SQLITE_OK; - - if (fg & (MEM_Str | MEM_Blob)) + if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0) return SQLITE_OK; assert(!(fg & MEM_Zero)); - assert(!(fg & (MEM_Str | MEM_Blob))); assert(fg & (MEM_Int | MEM_Real)); assert(EIGHT_BYTE_ALIGNMENT(pMem)); @@ -532,7 +528,7 @@ sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced) if (sqlite3VdbeIntValue(pMem, &i) == 0) { pMem->u.i = i; MemSetTypeFlag(pMem, MEM_Int); - return SQLITE_OK; + return 0; } else if ((pMem->flags & MEM_Real) != 0 && is_forced) { pMem->u.i = (int) pMem->u.r; MemSetTypeFlag(pMem, MEM_Int); @@ -545,7 +541,7 @@ sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced) } pMem->u.i = (int64_t) d; MemSetTypeFlag(pMem, MEM_Int); - return SQLITE_OK; + return 0; } /* @@ -606,9 +602,7 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff) { if (pMem->flags & MEM_Null) return SQLITE_OK; - if (pMem->flags & MEM_Blob && aff == AFFINITY_INTEGER) - return sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, pMem->n); - if (pMem->flags & MEM_Blob && + if ((pMem->flags & MEM_Blob) != 0 && (aff == AFFINITY_REAL || aff == AFFINITY_NUMERIC)) { if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, pMem->n) == 0) { MemSetTypeFlag(pMem, MEM_Real); @@ -631,6 +625,10 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff) case AFFINITY_NUMERIC: return sqlite3VdbeMemNumerify(pMem); case AFFINITY_INTEGER: + if ((pMem->flags & MEM_Blob) != 0) { + return sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, + pMem->n); + } return sqlite3VdbeMemIntegerify(pMem, true); case AFFINITY_REAL: return sqlite3VdbeMemRealify(pMem);