[tarantool-patches] Re: [PATCH 1/1] sql: IS is only applicable when dealing with NULL
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 24 14:52:31 MSK 2018
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.
>
>
More information about the Tarantool-patches
mailing list