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

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