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

[Nikita Tatunov <n.tatunov@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 8698 bytes --]

Hello! thank you for the review.

Issues:
https://github.com/tarantool/tarantool/issues/3251 <https://github.com/tarantool/tarantool/issues/3251>
https://github.com/tarantool/tarantool/issues/3334 <https://github.com/tarantool/tarantool/issues/3334>

Branch:
https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3251-where-like-hangs <https://github.com/tarantool/tarantool/tree/N_Tatunov/gh-3251-where-like-hangs>

Please consider following changes:

1. I rebased the whole branch on fresh 2.1.

> On Oct 29, 2018, at 15:15, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> Hi!
> 
> See remaining comments below.
> 
> WBR, Alexander Turenko.
> 
>>>> +static const int case_insensitive_like = 1;
>>>> +static const int case_sensitive_like = 0;
>>> 
>>> Are not they are mutually exclusive? Can we leave just one?
>>> 
>>> Why they are not defines?
>> 
>> I guess it’s a bad idea since we need pointers to them in some functions.
>> e.g:
>> 
>> 	int *is_like_ci;
>> 	if (is_case_sensitive)
>> 		is_like_ci = (void *)&case_sensitive_like;
>> 	else
>> 		is_like_ci = (void *)&case_insensitive_like;
>> 	sqlite3CreateFunc(db, "LIKE", 2, 0, is_like_ci, likeFunc, 0, 0, 0);
>> 	sqlite3CreateFunc(db, "LIKE", 3, 0, is_like_ci, likeFunc, 0, 0, 0);
> 
> I think we can use SQLITE_INT_TO_PTR and SQLITE_PTR_TO_INT to pass an
> integer value as a pointer and then cast it back. Maybe we also should
> change LIKEFUNC macro.
> 
>>>> +       int *is_like_ci;
>>>> +       if (is_case_sensitive)
>>>> +               is_like_ci = (int *)&case_sensitive_like;
>>>> +       else
>>>> +               is_like_ci = (int *)&case_insensitive_like;
>>> 
>>> Discarding const qualifier? It seems not being right thing to do. It is
>>> better to define is_like_ci pointer as const.
>> 
>> It’s not possible due to so-called “down qualification” and will cause
>> compiler warnings.
> 
> We'll anyway discard it here or there, so nevermind. I proposed to
> change it to SQLITE_INT_TO_PTR.
> 


2. Changed manipulations with “sensitiveness” of LIKE using your advice
thus there’s no static consts in func.c anymore.


diff with the previous version of the patch:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index b1983e594..5feab52f6 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -627,18 +627,6 @@ total_changes(sqlite3_context * context, int NotUsed, sqlite3_value ** NotUsed2)
 #define SQL_END_OF_STRING        0xffff
 #define SQL_INVALID_UTF8_SYMBOL  0xfffd
 
-/**
- * If SQLITE_CASE_SENSITIVE_LIKE is not defined, then the LIKE
- * operator is not case sensitive.
- */
-static const int case_insensitive_like = 1;
-
-/**
- * If SQLITE_CASE_SENSITIVE_LIKE is defined, then the LIKE
- * operator is case sensitive causing 'a' LIKE 'A' to be false.
- */
-static const int case_sensitive_like = 0;
-
 /**
  * Possible error returns from sql_utf8_pattern_compare().
  */
@@ -819,8 +807,7 @@ sql_utf8_pattern_compare(const char *pattern,
 int
 sql_strlike_cs(const char *zPattern, const char *zStr, unsigned int esc)
 {
-	return sql_utf8_pattern_compare(zPattern, zStr, case_sensitive_like,
-					esc);
+	return sql_utf8_pattern_compare(zPattern, zStr, 0, esc);
 }
 
 /**
@@ -830,8 +817,7 @@ sql_strlike_cs(const char *zPattern, const char *zStr, unsigned int esc)
 int
 sql_strlike_ci(const char *zPattern, const char *zStr, unsigned int esc)
 {
-	return sql_utf8_pattern_compare(zPattern, zStr, case_insensitive_like,
-					esc);
+	return sql_utf8_pattern_compare(zPattern, zStr, 1, esc);
 }
 
 /**
@@ -859,7 +845,7 @@ likeFunc(sqlite3_context *context, int argc, sqlite3_value **argv)
 	u32 escape = SQL_END_OF_STRING;
 	int nPat;
 	sqlite3 *db = sqlite3_context_db_handle(context);
-	int *is_like_ci = sqlite3_user_data(context);
+	int is_like_ci = SQLITE_PTR_TO_INT(sqlite3_user_data(context));
 
 #ifdef SQLITE_LIKE_DOESNT_MATCH_BLOBS
 	if (sqlite3_value_type(argv[0]) == SQLITE_BLOB
@@ -913,7 +899,7 @@ likeFunc(sqlite3_context *context, int argc, sqlite3_value **argv)
 	sqlite3_like_count++;
 #endif
 	int res;
-	res = sql_utf8_pattern_compare(zB, zA, *is_like_ci, escape);
+	res = sql_utf8_pattern_compare(zB, zA, is_like_ci, escape);
 	if (res == SQL_INVALID_PATTERN) {
 		const char *const err_msg =
 			"LIKE pattern can only contain UTF-8 characters";
@@ -1698,24 +1684,20 @@ setLikeOptFlag(sqlite3 * db, const char *zName, u8 flagVal)
  * @retval none.
  */
 void
-sqlite3RegisterLikeFunctions(sqlite3 *db, int is_case_sensitive)
+sqlite3RegisterLikeFunctions(sqlite3 *db, int *is_case_insensitive)
 {
 	/*
 	 * FIXME: after introducing type <BOOLEAN> LIKE must
 	 * return that type: TRUE if the string matches the
 	 * supplied pattern and FALSE otherwise.
 	 */
-	int *is_like_ci;
-	if (is_case_sensitive)
-		is_like_ci = (void *)&case_sensitive_like;
-	else
-		is_like_ci = (void *)&case_insensitive_like;
-	sqlite3CreateFunc(db, "LIKE", AFFINITY_INTEGER, 2, 0, is_like_ci,
-			  likeFunc, 0, 0, 0);
-	sqlite3CreateFunc(db, "LIKE", AFFINITY_INTEGER, 3, 0, is_like_ci,
-			  likeFunc, 0, 0, 0);
+	int is_like_ci = SQLITE_PTR_TO_INT(is_case_insensitive);
+	sqlite3CreateFunc(db, "LIKE", AFFINITY_INTEGER, 2, 0,
+			  is_case_insensitive, likeFunc, 0, 0, 0);
+	sqlite3CreateFunc(db, "LIKE", AFFINITY_INTEGER, 3, 0,
+			  is_case_insensitive, likeFunc, 0, 0, 0);
 	setLikeOptFlag(db, "LIKE",
-		       is_case_sensitive ? (SQLITE_FUNC_LIKE |
+		       !(is_like_ci) ? (SQLITE_FUNC_LIKE |
 		       SQLITE_FUNC_CASE) : SQLITE_FUNC_LIKE);
 }
 
@@ -1841,14 +1823,14 @@ sqlite3RegisterBuiltinFunctions(void)
 			  groupConcatFinalize, AFFINITY_TEXT),
 
 #ifdef SQLITE_CASE_SENSITIVE_LIKE
-		LIKEFUNC(like, 2, &case_sensitive_like,
-			 SQLITE_FUNC_LIKE | SQLITE_FUNC_CASE, AFFINITY_INTEGER),
-		LIKEFUNC(like, 3, &case_sensitive_like,
-			 SQLITE_FUNC_LIKE | SQLITE_FUNC_CASE, AFFINITY_INTEGER),
+		LIKEFUNC(like, 2, 0,SQLITE_FUNC_LIKE | SQLITE_FUNC_CASE,
+			 AFFINITY_INTEGER),
+		LIKEFUNC(like, 3, 0,SQLITE_FUNC_LIKE | SQLITE_FUNC_CASE,
+			 AFFINITY_INTEGER),
 #else
-		LIKEFUNC(like, 2, &case_insensitive_like, SQLITE_FUNC_LIKE,
+		LIKEFUNC(like, 2, 1, SQLITE_FUNC_LIKE,
 			AFFINITY_INTEGER),
-		LIKEFUNC(like, 3, &case_insensitive_like, SQLITE_FUNC_LIKE,
+		LIKEFUNC(like, 3, 1, SQLITE_FUNC_LIKE,
 			AFFINITY_INTEGER),
 #endif
 #ifdef SQLITE_ENABLE_UNKNOWN_SQL_FUNCTION
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index cd6db2b34..546b18ae2 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -586,9 +586,10 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
 		 */
 	case PragTyp_CASE_SENSITIVE_LIKE:{
 			if (zRight) {
+				int *is_like_ci =
+					SQLITE_INT_TO_PTR(!(sqlite3GetBoolean(zRight, 0)));
 				sqlite3RegisterLikeFunctions(db,
-							     sqlite3GetBoolean
-							     (zRight, 0));
+							     is_like_ci);
 			}
 			break;
 		}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index f92d4e6b8..dd819622b 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1759,7 +1759,7 @@ struct FuncDestructor {
    pArg, 0, xFunc, 0, #zName, {SQLITE_AFF_STRING, {0}}}
 #define LIKEFUNC(zName, nArg, arg, flags, type) \
   {nArg, SQLITE_FUNC_CONSTANT|flags, \
-   (void *)arg, 0, likeFunc, 0, #zName, {0}, type }
+   (void *)(SQLITE_INT_TO_PTR(arg)), 0, likeFunc, 0, #zName, {0}, type }
 #define AGGREGATE(zName, nArg, arg, nc, xStep, xFinal, type) \
   {nArg, (nc*SQLITE_FUNC_NEEDCOLL), \
    SQLITE_INT_TO_PTR(arg), 0, xStep,xFinal,#zName, {0}, type}
@@ -4551,7 +4551,7 @@ sql_key_info_unref(struct sql_key_info *key_info);
 struct key_def *
 sql_key_info_to_key_def(struct sql_key_info *key_info);
 
-void sqlite3RegisterLikeFunctions(sqlite3 *, int);
+void sqlite3RegisterLikeFunctions(sqlite3 *, int *);
 int sql_is_like_func(sqlite3 *, Expr *, int *);
 int sqlite3CreateFunc(sqlite3 *, const char *, enum affinity_type,
 		      int, int, void *,
diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
index eb4f43a90..dbbe1c0fe 100755
--- a/test/sql-tap/collation.test.lua
+++ b/test/sql-tap/collation.test.lua
@@ -219,7 +219,7 @@ local like_testcases =
         {0, {"Aab", "aaa"}} },
     {"2.1.2",
         "EXPLAIN QUERY PLAN SELECT * FROM tx1 WHERE s1 LIKE 'A%';",
-        {0, {0, 0, 0, "/USING COVERING INDEX I1/"}} },
+        {0, {0, 0, 0, "SEARCH TABLE TX1 USING COVERING INDEX I1 (S1>? AND S1<?)"}}},
     {"2.2.0",
         "PRAGMA case_sensitive_like = true;",
         {0}},


[-- Attachment #2: Type: text/html, Size: 19004 bytes --]