From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 0E6D7256FC for ; Fri, 18 May 2018 07:55:06 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id G9bA7XxRnAD3 for ; Fri, 18 May 2018 07:55:05 -0400 (EDT) Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id BE119256F7 for ; Fri, 18 May 2018 07:55:05 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL References: <1526642787.218420559@f261.i.mail.ru> From: Vladislav Shpilevoy Message-ID: <10ce4eb3-e380-3007-8811-78d6f4517e1b@tarantool.org> Date: Fri, 18 May 2018 14:55:03 +0300 MIME-Version: 1.0 In-Reply-To: <1526642787.218420559@f261.i.mail.ru> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, =?UTF-8?B?0JzQtdGA0LPQtdC9INCY0LzQtdC1?= =?UTF-8?B?0LI=?= 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: > From: Mergen Imeev > 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. > 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,@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. > > -%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. > -  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( >          -- >      }) > > -test:do_execsql_test( > -    "e_expr-8.1.3", > -    [[ > -        SELECT NULL IS     'ab' > -    ]], { > -        -- > -        0 > -        -- > -    }) > - > -test:do_execsql_test( > -    "e_expr-8.1.4", > -    [[ > -        SELECT 'ab' IS     'ab' > -    ]], { > -        -- > -        1 > -        -- > -    }) > +-- 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. > 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( >          -- >      }) > > -test:do_execsql_test( > -    "transitive1-401", > -    [[ > -        SELECT '1-row' FROM t401 LEFT JOIN t402 ON b IS a JOIN t403 ON c=a; > -    ]], { > -        -- > -        "1-row" > -        -- > -    }) 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 '=' ? > - > -test:do_execsql_test( > -    "transitive1-402", > -    [[ > -        SELECT '1-row' FROM t401 LEFT JOIN t402 ON b=a JOIN t403 ON c IS a; > -    ]], { > -        -- > -        "1-row" > -        -- > -    }) > - > -test:do_execsql_test( > -    "transitive1-403", > -    [[ > -        SELECT '1-row' FROM t401 LEFT JOIN t402 ON b IS a JOIN t403 ON c IS a; > -    ]], { > -        -- > -        "1-row" > -        -- > -    }) > -