[tarantool-patches] Re: [tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL
Мерген Имеев
imeevma at tarantool.org
Fri May 18 19:10:40 MSK 2018
>Пятница, 18 мая 2018, 14:55 +03:00 от Vladislav Shpilevoy <v.shpilevoy at tarantool.org>:
>
>Hello. Thanks for the patch! It is almost perfect! But look at my 5 comments below.
>
>On 18/05/2018 14:26, Мерген Имеев wrote:
>> From aa5528cfdd54def0a23c6fd8223d283fc834a1d6 Mon Sep 17 00:00:00 2001
>> Message-Id: < aa5528cfdd54def0a23c6fd8223d283fc834a1d6.1526478195.git.imeevma at tarantool.org >
>> From: Mergen Imeev < imeevma at tarantool.org >
>> Date: Wed, 16 May 2018 16:39:26 +0300
>
>1. Why do I see this in the message body? How do you send mail? Manually copy-pasting into
>your mail client? Or via git send-mail? Please, use git send-mail. SMTP headers must not be
>visible in the message body.
Understood. Will do next time.
>
>
>> Subject: [PATCH 1/1] sql: IS is only applicable when dealing with NULL
>>
>> According to ANSI Standard IS/IS NOT can be used to determine
>> if values is null. At the same time in SQLite3 IS/IS NOT have an
>> additional function - it can be used to check equality of two
>> values. This patch removes that additional function.
>>
>> Closes #2136
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/gh-2136-behaviour-of-IS-corrected
>> Issue: https://github.com/tarantool/tarantool/issues/2136
>>
>> src/box/sql/parse.y | 30 +---
>> test/sql-tap/e_expr.test.lua | 366 ++++++++++++++------------------------
>> test/sql-tap/lua_sql.test.lua | 12 +-
>> test/sql-tap/null.test.lua | 89 ++++++++-
>> test/sql-tap/subselect.test.lua | 2 +-
>> test/sql-tap/transitive1.test.lua | 45 +----
>> test/sql-tap/types.test.lua | 2 +-
>> test/sql-tap/types2.test.lua | 2 +-
>> 8 files changed, 229 insertions(+), 319 deletions(-)
>>
>> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>> index 872647d..4ad5195 100644
>> --- a/src/box/sql/parse.y
>> +++ b/src/box/sql/parse.y
>> @@ -1032,36 +1032,10 @@ expr(A) ::= expr(A) likeop(OP) expr(Y) ESCAPE expr(E). [LIKE_KW] {
>> }
>> }
>>
>> -expr(A) ::= expr(A) ISNULL|NOTNULL(E). {spanUnaryPostfix(pParse, at E,&A,&E);}
>> +expr(A) ::= expr(A) IS NULL(E). {spanUnaryPostfix(pParse,TK_ISNULL,&A,&E);}
>> +expr(A) ::= expr(A) IS NOT NULL(E). {spanUnaryPostfix(pParse,TK_NOTNULL,&A,&E);}
>> expr(A) ::= expr(A) NOT NULL(E). {spanUnaryPostfix(pParse,TK_NOTNULL,&A,&E);}
>
>2. What is 'NOT NULL' with no 'IS'. Is it allowed by the standard? Lets remove it too,
>if not.
Done
>
>
>>
>> -%include {
>> - /* A routine to convert a binary TK_IS or TK_ISNOT expression into a
>> - ** unary TK_ISNULL or TK_NOTNULL expression. */
>
>3. I still can grep ISNULL token. NOTNULL too. They must be removed completely, but
>very accurately. For example, TK_NULL, TK_ISNULL, TK_NOTNULL are not tokens, they
>are identifiers for 'NULL', 'IS NULL', and 'IS NOT NULL'. But strings 'ISNULL' and
>'NOTNULL' must be removed. For example, look extra/mkkeywordhash.c:196. Here all the
>tokens are stored and their identifiers, so "NOTNULL" is reflected into TK_NOTNULL.
>We must remove "NOTNULL". Same in parse.y:137, 213.
>
>In treeview.c:475 NOTNULL must be replaced with "NOT NULL". Same in vdbeaux.c:1508.
>
>Same about ISNULL.
Partially done. Ones in parse.y used to generate identifiers to operations 'IS NULL' and 'IS NOT NULL'
>
>
>> - static void binaryToUnaryIfNull(Parse *pParse, Expr *pY, Expr *pA, int op){
>> - sqlite3 *db = pParse->db;
>> - if( pA && pY && pY->op==TK_NULL ){
>> - pA->op = (u8)op;
>> - sql_expr_free(db, pA->pRight, false);
>> - pA->pRight = 0;
>> - }
>> - }
>> -}
>> -
>> -// expr1 IS expr2
>> -// expr1 IS NOT expr2
>> -//
>> -// If expr2 is NULL then code as TK_ISNULL or TK_NOTNULL. If expr2
>> -// is any other expression, code as TK_IS or TK_ISNOT.
>> -//
>> -expr(A) ::= expr(A) IS expr(Y). {
>> - spanBinaryExpr(pParse,TK_IS,&A,&Y);
>> - binaryToUnaryIfNull(pParse, Y.pExpr, A.pExpr, TK_ISNULL);
>> -}
>> -expr(A) ::= expr(A) IS NOT expr(Y). {
>> - spanBinaryExpr(pParse,TK_ISNOT,&A,&Y);
>> - binaryToUnaryIfNull(pParse, Y.pExpr, A.pExpr, TK_NOTNULL);
>> -}
>>
>> %include {
>> /* Construct an expression node for a unary prefix operator
>> diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
>> index d0f6895..d378222 100755
>> --- a/test/sql-tap/e_expr.test.lua
>> +++ b/test/sql-tap/e_expr.test.lua
>> @@ -512,25 +512,27 @@ test:do_execsql_test(
>> -- </e_expr-8.1.2>
>> })
>>
>> -test:do_execsql_test(
>> - "e_expr-8.1.3",
>> - [[
>> - SELECT NULL IS 'ab'
>> - ]], {
>> - -- <e_expr-8.1.3>
>> - 0
>> - -- </e_expr-8.1.3>
>> - })
>> -
>> -test:do_execsql_test(
>> - "e_expr-8.1.4",
>> - [[
>> - SELECT 'ab' IS 'ab'
>> - ]], {
>> - -- <e_expr-8.1.4>
>> - 1
>> - -- </e_expr-8.1.4>
>> - })
>> +-- gh-2136: According to ANSI SQL IS can be used only in IS NULL or IS NOT NULL
>
>4. Please, remove the old tests, not comment them.
Done
>
>> diff --git a/test/sql-tap/transitive1.test.lua b/test/sql-tap/transitive1.test.lua
>> index bdb9e97..ed3238f 100755
>> --- a/test/sql-tap/transitive1.test.lua
>> +++ b/test/sql-tap/transitive1.test.lua
>> @@ -234,36 +221,6 @@ test:do_execsql_test(
>> -- </transitive1-400>
>> })
>>
>> -test:do_execsql_test(
>> - "transitive1-401",
>> - [[
>> - SELECT '1-row' FROM t401 LEFT JOIN t402 ON b IS a JOIN t403 ON c=a;
>> - ]], {
>> - -- <transitive1-401>
>> - "1-row"
>> - -- </transitive1-401>
>> - })
>
>5. Are you sure these tests must be removed? Looks like they test
>JOINs, not IS. Maybe it is better to fix them replacing IS with '=' ?
There were four of pretty much identical tests. I deleted ones that used 'IS' as replacement to '='. One left
>
>
>> -
>> -test:do_execsql_test(
>> - "transitive1-402",
>> - [[
>> - SELECT '1-row' FROM t401 LEFT JOIN t402 ON b=a JOIN t403 ON c IS a;
>> - ]], {
>> - -- <transitive1-402>
>> - "1-row"
>> - -- </transitive1-402>
>> - })
>> -
>> -test:do_execsql_test(
>> - "transitive1-403",
>> - [[
>> - SELECT '1-row' FROM t401 LEFT JOIN t402 ON b IS a JOIN t403 ON c IS a;
>> - ]], {
>> - -- <transitive1-403>
>> - "1-row"
>> - -- </transitive1-403>
>> - })
>> -
>
Result of "git show" contains more than 14 000 lines of text.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180518/f2498375/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.txt
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20180518/f2498375/attachment.txt>
More information about the Tarantool-patches
mailing list