From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 126B324E92 for ; Thu, 24 May 2018 15:46:43 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id BiaF0UqiSQbh for ; Thu, 24 May 2018 15:46:42 -0400 (EDT) Received: from smtp5.mail.ru (smtp5.mail.ru [94.100.179.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C390124E87 for ; Thu, 24 May 2018 15:46:42 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL References: <1526642787.218420559@f261.i.mail.ru> <10ce4eb3-e380-3007-8811-78d6f4517e1b@tarantool.org> <1526659840.883327245@f345.i.mail.ru> <7283762e-ce01-fd16-a50b-26b6730711cc@tarantool.org> <6984db02-b1d1-942b-6a04-8684fcbc87ae@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 24 May 2018 22:46:38 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Imeev Mergen , Kirill Yukhin 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. >>> >>> >