[tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Nov 2 14:15:22 MSK 2018
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 <v.shpilevoy at tarantool.org> 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(
> -- </cast-1.4>
> })
>
> -if false then
> -test:do_execsql_test(
> +test:do_catchsql_test(
> "cast-1.5",
> [[
> SELECT CAST(x'616263' AS numeric)
> ]], {
> -- <cast-1.5>
> - 0
> + 1, 'Type mismatch: can not convert abc to real'
> -- </cast-1.5>
> })
>
> -test:do_execsql_test(
> - "cast-1.6",
> - [[
> - SELECT typeof(CAST(x'616263' AS numeric))
> - ]], {
> - -- <cast-1.6>
> - "real"
> - -- </cast-1.6>
> - })
> -end
> -
> test:do_execsql_test(
> "cast-1.7",
> [[
> @@ -106,29 +94,16 @@ test:do_execsql_test(
> -- </cast-1.8>
> })
>
> -if false then
> -test:do_execsql_test(
> +test:do_catchsql_test(
> "cast-1.9",
> [[
> SELECT CAST(x'616263' AS integer)
> ]], {
> -- <cast-1.9>
> - 0
> + 1, 'Type mismatch: can not convert abc to integer'
> -- </cast-1.9>
> })
>
> -test:do_execsql_test(
> - "cast-1.10",
> - [[
> - SELECT typeof(CAST(x'616263' AS integer))
> - ]], {
> - -- <cast-1.10>
> -if false then
> -test:do_execsql_test(
> +test:do_catchsql_test(
> "cast-1.5",
> [[
> SELECT CAST(x'616263' AS numeric)
> ]], {
> -- <cast-1.5>
> - 0
> + 1, 'Type mismatch: can not convert abc to real'
> -- </cast-1.5>
> })
>
> -test:do_execsql_test(
> - "cast-1.6",
> - [[
> - SELECT typeof(CAST(x'616263' AS numeric))
> - ]], {
> - -- <cast-1.6>
> - "real"
> - -- </cast-1.6>
> - })
> -end
> -
> test:do_execsql_test(
> "cast-1.7",
> [[
> @@ -106,29 +94,16 @@ test:do_execsql_test(
> -- </cast-1.8>
> })
>
> -if false then
> -test:do_execsql_test(
> +test:do_catchsql_test(
> "cast-1.9",
> [[
> SELECT CAST(x'616263' AS integer)
> ]], {
> -- <cast-1.9>
> - 0
> + 1, 'Type mismatch: can not convert abc to integer'
> -- </cast-1.9>
> })
>
> -test:do_execsql_test(
> - "cast-1.10",
> - [[
> - SELECT typeof(CAST(x'616263' AS integer))
> - ]], {
> - -- <cast-1.10>
> - "integer"
> - -- </cast-1.10>
> - })
> -end
> -
> -
> test:do_execsql_test(
> "cast-1.11",
> [[
> @@ -289,7 +264,6 @@ test:do_execsql_test(
> -- </cast-1.26>
> })
>
> -if false then
> test:do_execsql_test(
> "cast-1.27",
> [[
> @@ -309,7 +283,6 @@ test:do_execsql_test(
> "blob"
> -- </cast-1.28>
> })
> -end
>
> test:do_execsql_test(
> "cast-1.29",
> @@ -391,7 +364,6 @@ test:do_execsql_test(
> -- </cast-1.36>
> })
>
> -if false then
> test:do_execsql_test(
> "cast-1.37",
> [[
> @@ -411,7 +383,6 @@ test:do_execsql_test(
> "blob"
> -- </cast-1.38>
> })
> -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.
>
More information about the Tarantool-patches
mailing list