[tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri May 18 14:55:03 MSK 2018
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.
> 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.
>
> -%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>
> - })
> -
More information about the Tarantool-patches
mailing list