From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: "n.pettik" <korablev@tarantool.org>, tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions
Date: Fri, 2 Nov 2018 14:15:22 +0300 [thread overview]
Message-ID: <6aef2220-8320-0fbf-d051-100bf7d95f17@tarantool.org> (raw)
In-Reply-To: <4BC8591A-1071-4D26-B18F-F0893E95C7EA@tarantool.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 <v.shpilevoy@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.
>
next prev parent reply other threads:[~2018-11-02 11:15 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-17 20:32 [tarantool-patches] [PATCH 0/6] Introduce strict typing for SQL Nikita Pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 1/6] sql: split conflict action and affinity for Expr Nikita Pettik
2018-09-19 2:16 ` [tarantool-patches] " Konstantin Osipov
2018-09-27 20:24 ` Vladislav Shpilevoy
2018-10-12 11:18 ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 2/6] sql: annotate SQL functions with return type Nikita Pettik
2018-09-27 20:23 ` [tarantool-patches] " Vladislav Shpilevoy
2018-10-12 11:18 ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 3/6] sql: pass true types of columns to Tarantool Nikita Pettik
2018-09-19 2:23 ` [tarantool-patches] " Konstantin Osipov
2018-10-12 11:19 ` n.pettik
2018-09-27 20:23 ` Vladislav Shpilevoy
2018-10-12 11:18 ` n.pettik
2018-10-17 21:45 ` Vladislav Shpilevoy
2018-10-23 23:28 ` n.pettik
2018-10-29 21:32 ` Vladislav Shpilevoy
2018-11-02 2:36 ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 4/6] sql: enforce implicit type conversions Nikita Pettik
2018-09-19 2:25 ` [tarantool-patches] " Konstantin Osipov
2018-09-27 20:24 ` Vladislav Shpilevoy
2018-10-12 11:19 ` n.pettik
2018-10-17 21:45 ` Vladislav Shpilevoy
2018-10-23 23:28 ` n.pettik
2018-10-29 21:32 ` Vladislav Shpilevoy
2018-11-02 2:36 ` n.pettik
2018-11-02 11:15 ` Vladislav Shpilevoy [this message]
2018-11-02 13:26 ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 5/6] sql: return result-set type via IProto Nikita Pettik
2018-09-19 2:26 ` [tarantool-patches] " Konstantin Osipov
2018-09-27 20:24 ` Vladislav Shpilevoy
2018-10-12 11:19 ` n.pettik
2018-10-17 21:45 ` Vladislav Shpilevoy
2018-10-23 23:28 ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 6/6] sql: discard numeric conversion by unary plus Nikita Pettik
2018-09-27 20:24 ` [tarantool-patches] " Vladislav Shpilevoy
2018-10-12 11:19 ` n.pettik
2018-09-27 20:24 ` [tarantool-patches] Re: [PATCH 0/6] Introduce strict typing for SQL Vladislav Shpilevoy
2018-10-12 11:18 ` n.pettik
2018-11-03 2:41 ` Kirill Yukhin
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=6aef2220-8320-0fbf-d051-100bf7d95f17@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions' \
/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