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 95CCE2EF4C for ; Fri, 2 Nov 2018 07:15:29 -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 f2rmKUKs2Tid for ; Fri, 2 Nov 2018 07:15:29 -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 29B852EF49 for ; Fri, 2 Nov 2018 07:15:27 -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> <999eb301-5c83-f1d8-491d-87c52ab5815f@tarantool.org> <4BC8591A-1071-4D26-B18F-F0893E95C7EA@tarantool.org> From: Vladislav Shpilevoy Message-ID: <6aef2220-8320-0fbf-d051-100bf7d95f17@tarantool.org> Date: Fri, 2 Nov 2018 14:15:22 +0300 MIME-Version: 1.0 In-Reply-To: <4BC8591A-1071-4D26-B18F-F0893E95C7EA@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: "n.pettik" , tarantool-patches@freelists.org Hi! Thanks for the patch! See a couple of style micro fixes on the branch and below: ===================================================== diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 3111c76e6..ab07e7c1c 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -330,7 +330,7 @@ static int mem_apply_affinity(struct Mem *record, enum affinity_type affinity) { if ((record->flags & MEM_Null) != 0) - return SQLITE_OK; + return 0; switch (affinity) { case AFFINITY_INTEGER: if ((record->flags & MEM_Int) == MEM_Int) diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c index ad9b33b99..22beba8be 100644 --- a/src/box/sql/vdbemem.c +++ b/src/box/sql/vdbemem.c @@ -631,7 +631,7 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff) case AFFINITY_INTEGER: if ((pMem->flags & MEM_Blob) != 0) { if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, - pMem->n) != 0) + pMem->n) != 0) return -1; MemSetTypeFlag(pMem, MEM_Int); return 0; ===================================================== Otherwise the patchset LGTM 🎉 On 02/11/2018 05:36, n.pettik wrote: > > >> On 30 Oct 2018, at 00:32, Vladislav Shpilevoy wrote: >> >> Thanks for the fixes! See 3 comments below. >> >>> --- a/test/sql-tap/boundary1.test.lua >>> +++ b/test/sql-tap/boundary1.test.lua >>> @@ -7649,5 +7649,4 @@ test:do_execsql_test( >>> "boundary1-2.66.le.5", >>> "SELECT a FROM t1 WHERE rowid <= -9.22337303685477580800e+18 ORDER BY x", >>> {}) >>> - >>> test:finish_test( >> 1. Stray diff in boundary1.test.lua. > > Removed. > >>>>> 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. >>> I did it in boundary3 test and forgot about this one. Fixed: >> >> 2. Sorry, still can be found in cast.test.lua. > > Thx, I found another one bug with CAST operator. > Here is the fix. I also uncommented the rest of tests. > In a nutshell: CAST(x’100’ as INT) and CAST(123 as BLOB) > didn’t work since I forgot to set a flag to memory cell. > Moreover, sqlite3AtoF returns 1 (true) in case of success, > so we need to revert its return code. > > +++ b/src/box/sql/vdbemem.c > @@ -609,7 +609,7 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff) > pMem->u.r = pMem->u.i; > return 0; > } > - return sqlite3AtoF(pMem->z, &pMem->u.r, pMem->n); > + return ! sqlite3AtoF(pMem->z, &pMem->u.r, pMem->n); > } > switch (aff) { > case AFFINITY_BLOB: > @@ -619,15 +619,22 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff) > MemSetTypeFlag(pMem, MEM_Blob); > return SQLITE_OK; > } > - if (pMem->flags & MEM_Int || pMem->flags & MEM_Real) > - return sqlite3VdbeMemStringify(pMem, 0); > + if (pMem->flags & MEM_Int || pMem->flags & MEM_Real) { > + if (sqlite3VdbeMemStringify(pMem, 1) != 0) > + return -1; > + MemSetTypeFlag(pMem, MEM_Blob); > + return 0; > + } > return SQLITE_ERROR; > 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); > + if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, > + pMem->n) != 0) > + return -1; > + MemSetTypeFlag(pMem, MEM_Int); > + return 0; > } > return sqlite3VdbeMemIntegerify(pMem, true); > case AFFINITY_REAL: > > diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua > index ee132e11d..e57bdaf1d 100755 > --- a/test/sql-tap/cast.test.lua > +++ b/test/sql-tap/cast.test.lua > @@ -1,6 +1,6 @@ > #!/usr/bin/env tarantool > test = require("sqltester") > -test:plan(72) > +test:plan(82) > > --!./tcltestrunner.lua > -- 2005 June 25 > @@ -64,28 +64,16 @@ test:do_execsql_test( > -- > }) > > -if false then > -test:do_execsql_test( > +test:do_catchsql_test( > "cast-1.5", > [[ > SELECT CAST(x'616263' AS numeric) > ]], { > -- > - 0 > + 1, 'Type mismatch: can not convert abc to real' > -- > }) > > -test:do_execsql_test( > - "cast-1.6", > - [[ > - SELECT typeof(CAST(x'616263' AS numeric)) > - ]], { > - -- > - "real" > - -- > - }) > -end > - > test:do_execsql_test( > "cast-1.7", > [[ > @@ -106,29 +94,16 @@ test:do_execsql_test( > -- > }) > > -if false then > -test:do_execsql_test( > +test:do_catchsql_test( > "cast-1.9", > [[ > SELECT CAST(x'616263' AS integer) > ]], { > -- > - 0 > + 1, 'Type mismatch: can not convert abc to integer' > -- > }) > > -test:do_execsql_test( > - "cast-1.10", > - [[ > - SELECT typeof(CAST(x'616263' AS integer)) > - ]], { > - -- > -if false then > -test:do_execsql_test( > +test:do_catchsql_test( > "cast-1.5", > [[ > SELECT CAST(x'616263' AS numeric) > ]], { > -- > - 0 > + 1, 'Type mismatch: can not convert abc to real' > -- > }) > > -test:do_execsql_test( > - "cast-1.6", > - [[ > - SELECT typeof(CAST(x'616263' AS numeric)) > - ]], { > - -- > - "real" > - -- > - }) > -end > - > test:do_execsql_test( > "cast-1.7", > [[ > @@ -106,29 +94,16 @@ test:do_execsql_test( > -- > }) > > -if false then > -test:do_execsql_test( > +test:do_catchsql_test( > "cast-1.9", > [[ > SELECT CAST(x'616263' AS integer) > ]], { > -- > - 0 > + 1, 'Type mismatch: can not convert abc to integer' > -- > }) > > -test:do_execsql_test( > - "cast-1.10", > - [[ > - SELECT typeof(CAST(x'616263' AS integer)) > - ]], { > - -- > - "integer" > - -- > - }) > -end > - > - > test:do_execsql_test( > "cast-1.11", > [[ > @@ -289,7 +264,6 @@ test:do_execsql_test( > -- > }) > > -if false then > test:do_execsql_test( > "cast-1.27", > [[ > @@ -309,7 +283,6 @@ test:do_execsql_test( > "blob" > -- > }) > -end > > test:do_execsql_test( > "cast-1.29", > @@ -391,7 +364,6 @@ test:do_execsql_test( > -- > }) > > -if false then > test:do_execsql_test( > "cast-1.37", > [[ > @@ -411,7 +383,6 @@ test:do_execsql_test( > "blob" > -- > }) > -end > > test:do_execsql_test( > "cast-1.39", > @@ -786,7 +757,7 @@ test:do_execsql_test( > > > > -if false then --test:execsql("PRAGMA encoding")[1][1]=="UTF-8" then > +if true then --test:execsql("PRAGMA encoding")[1][1]=="UTF-8" then > test:do_execsql_test( > "cast-3.21", > [[ > >> >> Also stray empty lines on 526, 763. > > Removed. > >> >> 3. if false in in3.test.lua. > > Removed in previous commit. >