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 636BB24C0E for ; Thu, 24 May 2018 07:52:36 -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 AT3OpLba_yWy for ; Thu, 24 May 2018 07:52:36 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 16A2E210E4 for ; Thu, 24 May 2018 07:52:35 -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> From: Vladislav Shpilevoy Message-ID: <6984db02-b1d1-942b-6a04-8684fcbc87ae@tarantool.org> Date: Thu, 24 May 2018 14:52:31 +0300 MIME-Version: 1.0 In-Reply-To: <7283762e-ce01-fd16-a50b-26b6730711cc@tarantool.org> 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 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. > >