[tarantool-patches] Re: [PATCH 2/2] sql: remove GLOB from Tarantool

["n.pettik" <korablev@tarantool.org>]

> On 13 Nov 2018, at 22:23, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> Hi!
> 
> I gave some minor comments and I think I cannot add any value anymore.
> Nikita N., can you look into this patch?

>    Part of #3589
>    Part of #3572
>    Needed for #3251
>    Needed for #3334

Why this patch is ’needed for’ but comes after
these issues are closed by previous patch?

Also I don’t like the fact that your patch not only removes
GLOB function, but also changes some user-visible interface.
It still would be OK if you wrote about it in commit message
or in doc request. For instance, as I understand from code,
percent symbol (‘%’) now works as star (‘*’) did before.

Personally I don’t like many code style fixes because
they obfuscate patch and make it complicate to review.
What is more, they don’t deal with all problems: they
partially fix comments but don’t fix naming. It looks awful.
But since it is done and two reviewers already gave their
OK I guess it would be irrational to start from scratch.

Finally, to be honest I didn’t review ALL changed tests.
I looked through and they seem to be OK.

I’ve pushed also several non-functional fixes -
see branch np/gh-3251-where-like-hangs.

Let's assume this as formal LGTM.

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index f0baf3db0..c3d3ccfb2 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1674,12 +1674,6 @@ setLikeOptFlag(sqlite3 * db, const char *zName, u8 flagVal)
 
 /**
  * Register the built-in LIKE function.
- *
- * @param db database structure.
- * @param is_case_sensitive whether like should be case sensitive
- *                         or not.
- *
- * @retval none.
  */
 void
 sqlite3RegisterLikeFunctions(sqlite3 *db, int is_case_insensitive)
@@ -1699,30 +1693,18 @@ sqlite3RegisterLikeFunctions(sqlite3 *db, int is_case_insensitive)
                       SQLITE_FUNC_CASE) : SQLITE_FUNC_LIKE);
 }
 
-/**
- * Check if the function implements LIKE-style comparison & if it
- * is appropriate to apply a LIKE query optimization.
- *
- * @param db database structure.
- * @param pExpr pointer to a function-implementing expression.
- * @param is_like_ci true if LIKE is case insensitive.
- *
- * @retval 0 if it's appropriate to apply optimization.
- *         1 if it's not.
- */
 int
-sql_is_like_func(sqlite3 *db, Expr *pExpr, int *is_like_ci)
+sql_is_like_func(struct sqlite3 *db, struct Expr *expr, bool *is_like_ci)
 {
-       FuncDef *pDef;
-       if (pExpr->op != TK_FUNCTION || !pExpr->x.pList ||
-           pExpr->x.pList->nExpr != 2)
+       if (expr->op != TK_FUNCTION || !expr->x.pList ||
+           expr->x.pList->nExpr != 2)
                return 0;
-       assert(!ExprHasProperty(pExpr, EP_xIsSelect));
-       pDef = sqlite3FindFunction(db, pExpr->u.zToken, 2, 0);
-       if (NEVER(pDef == 0) || (pDef->funcFlags & SQLITE_FUNC_LIKE) == 0) {
+       assert(!ExprHasProperty(expr, EP_xIsSelect));
+       struct FuncDef *func = sqlite3FindFunction(db, expr->u.zToken, 2, 0);
+       assert(func != NULL);
+       if ((func->funcFlags & SQLITE_FUNC_LIKE) == 0)
                return 0;
-       }
-       *is_like_ci = (pDef->funcFlags & SQLITE_FUNC_CASE) == 0;
+       *is_like_ci = (func->funcFlags & SQLITE_FUNC_CASE) == 0;
        return 1;
 }
 
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 07e83e444..af124b731 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4552,7 +4552,20 @@ struct key_def *
 sql_key_info_to_key_def(struct sql_key_info *key_info);
 
 void sqlite3RegisterLikeFunctions(sqlite3 *, int);
-int sql_is_like_func(sqlite3 *, Expr *, int *);
+
+/**
+ * Check if the function implements LIKE-style comparison & if it
+ * is appropriate to apply a LIKE query optimization.
+ *
+ * @param db database structure.
+ * @param pExpr pointer to a function-implementing expression.
+ * @param[out] is_like_ci true if LIKE is case insensitive.
+ *
+ * @retval 1 if LIKE optimization can be used, 0 otherwise.
+ */
+int
+sql_is_like_func(struct sqlite3 *db, struct Expr *expr, bool *is_like_ci);
+
 int sqlite3CreateFunc(sqlite3 *, const char *, enum affinity_type,
                      int, int, void *,
                      void (*)(sqlite3_context *, int, sqlite3_value **),
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 5f6344405..5d6b04418 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -232,21 +232,17 @@ operatorMask(int op)
  * @param pParse      Parsing and code generating context.
  * @param pExpr       Test this expression.
  * @param ppPrefix    Pointer to TK_STRING expression with
- *                   pattern prefix.
+ *                    pattern prefix.
  * @param pisComplete True if the only wildcard is '%' in the
- *                   last character.
+ *                    last character.
  * @param pnoCase     True if case insensitive.
  *
  * @retval True if the given expr is a LIKE operator & is
- *        optimizable using inequality constraints.
- *        False if not.
+ *         optimizable using inequality constraints.
  */
 static int
-is_like(Parse *pParse,
-       Expr *pExpr,
-       Expr **ppPrefix,
-       int *pisComplete,
-       int *pnoCase)
+like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
+                          int *pisComplete, int *pnoCase)
 {
        /* String on RHS of LIKE operator. */
        const char *z = 0;
@@ -1158,17 +1154,16 @@ exprAnalyze(SrcList * pSrc,     /* the FROM clause */
         * is made all lowercase so that the bounds also work
         * when comparing BLOBs.
         */
-       if (pWC->op == TK_AND
-           && is_like(pParse, pExpr, &pStr1, &isComplete, &noCase)) {
-               /* LHS of LIKE operator */
+       if (pWC->op == TK_AND &&
+           like_optimization_is_valid(pParse, pExpr, &pStr1,
+                                      &isComplete, &noCase)) {
                Expr *pLeft;
-               /* Copy of pStr1 - RHS of LIKE operator */
+               /* Copy of pStr1 - RHS of LIKE operator. */
                Expr *pStr2;
                Expr *pNewExpr1;
                Expr *pNewExpr2;
                int idxNew1;
                int idxNew2;
-               /* Name of collating sequence */
                const char *zCollSeqName;
                const u16 wtFlags = TERM_LIKEOPT | TERM_VIRTUAL | TERM_DYNAMIC;
 
@@ -1179,7 +1174,7 @@ exprAnalyze(SrcList * pSrc,       /* the FROM clause */
                 * Convert the lower bound to upper-case and the
                 * upper bound to lower-case (upper-case is less
                 * than lower-case in ASCII) so that the range
-                * constraints also work for BLOBs
+                * constraints also work for BLOBs.
                 */
                if (noCase && !pParse->db->mallocFailed) {
                        int i;
diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
index f49810999..db44999a6 100755
--- a/test/sql-tap/e_expr.test.lua
+++ b/test/sql-tap/e_expr.test.lua
@@ -1264,7 +1264,6 @@ test:do_execsql_test(
 test:execsql [[
     CREATE TABLE tblname(cname INT PRIMARY KEY);
 ]]
-
 local function glob(args)
     return 1
 end