Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org, Imeev Mergen <imeevma@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL
Date: Thu, 24 May 2018 14:52:31 +0300	[thread overview]
Message-ID: <6984db02-b1d1-942b-6a04-8684fcbc87ae@tarantool.org> (raw)
In-Reply-To: <7283762e-ce01-fd16-a50b-26b6730711cc@tarantool.org>

Hello. Good patch! See my 5 minor comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 1b5182367..1ed8d1c40 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
@@ -564,8 +563,6 @@ exprVectorRegister(Parse * pParse,	/* Parse context */
   *
   * The caller must satisfy the following preconditions:
   *
- *    if pExpr->op==TK_IS:      op==TK_EQ and p5==SQLITE_NULLEQ
- *    if pExpr->op==TK_ISNOT:   op==TK_NE and p5==SQLITE_NULLEQ
   *    otherwise:                op==pExpr->op and p5==0
   */
  static void

1. Now the sentence looks weird: 'caller must satisfy the following preconditions otherwise'.
Please, fix it. And I noticed this: according to the comment now `op` is always
the same as `pExpr->op`, so this argument can be removed. Please, do it if possible.

> @@ -4924,6 +4906,9 @@ sqlite3ExprIfFalse(Parse * pParse, Expr * pExpr, int dest, int jumpIfNull)
>  	 */
>  	assert(pExpr->op != TK_ISNULL || op == OP_NotNull);
>  	assert(pExpr->op != TK_NOTNULL || op == OP_IsNull);
> +
> +	op = ((pExpr->op + (TK_NE & 1)) ^ 1) - (TK_NE & 1);
> +
>  	assert(pExpr->op != TK_NE || op == OP_Eq);
>  	assert(pExpr->op != TK_EQ || op == OP_Ne);
>  	assert(pExpr->op != TK_LT || op == OP_Ge);

2. Lets fix the comment as well so as to make make it clear TK_ISNULL/NOTNULL now
are not in the same range as TK_NE/EQ....

> @@ -126,7 +126,7 @@ cmdx ::= cmd.
>  // which keeps parser tables smaller.
>  //
>  // The token values assigned to these symbols is determined by the order
> -// in which lemon first sees them.  It must be the case that ISNULL/NOTNULL,
> +// in which lemon first sees them.  It must be the case that
>  // NE/EQ, GT/LE, and GE/LT are separated by only a single value.  See
>  // the sqlite3ExprIfFalse() routine for additional information on this
>  // constraint.

3. Please say a pair of words about how TK_NOTNULL/ISNULL work now.

> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
> index 6b63b2c69..f64eb72e4 100644
> --- a/src/box/sql/whereexpr.c
> +++ b/src/box/sql/whereexpr.c
> @@ -1051,8 +1047,6 @@ exprAnalyze(SrcList * pSrc,	/* the FROM clause */
>  			pTerm->u.leftColumn = iColumn;
>  			pTerm->eOperator = operatorMask(op) & opMask;
>  		}
> -		if (op == TK_IS)
> -			pTerm->wtFlags |= TERM_IS;
>  		if (pRight
>  		    && exprMightBeIndexed(pSrc, op, pTerm->prereqRight, pRight,
>  					  &iCur, &iColumn)

4. TERM_IS now is never set, and can be removed too.

> @@ -1252,7 +1244,7 @@ exprAnalyze(SrcList * pSrc,	/* the FROM clause */
>  	 * This is only required if at least one side of the comparison operation
>  	 * is not a sub-select.
>  	 */
> -	if (pWC->op == TK_AND && (pExpr->op == TK_EQ || pExpr->op == TK_IS)
> +	if (pWC->op == TK_AND && (pExpr->op == TK_EQ)
>  	    && sqlite3ExprIsVector(pExpr->pLeft)
>  	    && ((pExpr->pLeft->flags & EP_xIsSelect) == 0
>  		|| (pExpr->pRight->flags & EP_xIsSelect) == 0)) {

5. Here you can remove extra ():
(pExpr->op == TK_EQ) -> pExpr->op == TK_EQ

They were needed to encapsulate the disjunction.


On 23/05/2018 17:30, Imeev Mergen wrote:
> 
> 
> On 05/19/2018 12:37 AM, Vladislav Shpilevoy wrote:
>> Hello. Thanks for the fixes! You did a huge work fixing the tests!
>>
>>>      >
>>>      > -%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'
>>
>> I see, but they now are unused by parser. Please, remove them. The parser uses not "NOTNULL" or
>> "ISNULL" - it uses TK_ISNULL and TK_NOTNULL. Look at addopcodes.sh how to add TK_... constants
>> with no adding them to parser.
>>
>> In the final patch you must not be able to grep "ISNULL" or "NOTNULL" strings anywhere.
>>
>> By removal of these parser-unused things you will make the parser slightly faster.
>>
>>
>> The rest of the patch is ok.
> Done.
> 
> 

  reply	other threads:[~2018-05-24 11:52 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-18 11:26 [tarantool-patches] " Мерген Имеев
2018-05-18 11:55 ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-18 16:10   ` [tarantool-patches] " Мерген Имеев
2018-05-18 21:37     ` Vladislav Shpilevoy
2018-05-23 14:30       ` Imeev Mergen
2018-05-24 11:52         ` Vladislav Shpilevoy [this message]
2018-05-24 14:00           ` Imeev Mergen
2018-05-24 19:46             ` Vladislav Shpilevoy
2018-05-30  8:35               ` Kirill Yukhin
2018-05-18 14:34 ` Konstantin Osipov
  -- strict thread matches above, loose matches on Subject: below --
2018-05-16 13:50 [tarantool-patches] " Мерген Имеев
2018-05-16 18:52 ` [tarantool-patches] " Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6984db02-b1d1-942b-6a04-8684fcbc87ae@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox