[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