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.
>>
>>