>Пятница, 18 мая 2018, 14:55 +03:00 от Vladislav Shpilevoy : > >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@tarantool.org > >> From: Mergen Imeev < imeevma@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,@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( >>          -- >>      }) >> >> -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. 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( >>          -- >>      }) >> >> -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 '=' ? 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; >> -    ]], { >> -        -- >> -        "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" >> -        -- >> -    }) >> - > Result of "git show" contains more than 14 000 lines of text.