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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! I force pushed my review fixes. Now LGTM.

Kirill, please, do something.

On 24/05/2018 17:00, Imeev Mergen wrote:
> 
> 
> On 05/24/2018 02:52 PM, Vladislav Shpilevoy wrote:
>> 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.
> Done. Also removed "op" and "r5" arguments.
>>
>>> @@ -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....
> Done.
>>
>>> @@ -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.
> Done. Added couple lines of text before definition of IS NULL and
> IS NOT NULL operations.
>>
>>> 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.
> Done.
>>
>>> @@ -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.
> Done.
>>
>>
>> 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.
>>>
>>>
>