[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