[tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL

[Vladislav Shpilevoy <v.shpilevoy@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@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.

> 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(
>           -- </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.
> 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 '=' ?

> -
> -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>
> -    })
> -