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

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

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

Hello!

Thank you for comments.

> On Nov 9, 2018, at 15:18, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> Hi!
> 
> See comments below.
> 
> WBR, Alexander Turenko.
> 
> On Thu, Nov 08, 2018 at 06:09:23PM +0300, Nikita Tatunov wrote:
>> 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>
>> 
> 
> It seems you forgot to push the new changes.

Ermm, did I? I have just opened the branch and it contains a commit
which was pushed at 8th of November (and as I see it’s exactly the commit
from the diff)

> 
>> @@ -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);
>> }
> 
> I think sqlite3RegisterLikeFunctions should accept int as the second
> parameter.

Sure, fixed it.

> BTW, it seems that we add SQLITE_FUNC_CASE into flags, so maybe we can
> read it in likeFunc and don't pass additional parameter at all? It is
> just raw idea, I don't know the mechanics of sql functions much.

I’m not sure it’s a good idea, at least not in terms of this patch.
Call to likeFunc comes right from OP_FUNCTION which is used for sql
functions (of course it’s not only LIKE). Thus the list of parameters is
pretty much defined there.

	(*pCtx->pFunc->xSFunc)(pCtx, pCtx->argc, pCtx->argv);/* IMP: R-24505-23230 */

> 
>> 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<?)"}}},
> 
> What is this hunk about?

I found that this changed message occurs even on fresh 2.1.
I was using this and few other test cases to test my patch so it was
kind of concerned with the patch and i decided to fix it at the same time.
(I don’t really think it does worth opening an issue).
I assume this issue was caused by commit 6b8acd8fde and just
wasn’t fixed.
I guess I should’ve wrote about it in the previous letter.


diff with the previous version:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 5feab52f6..ed7926b18 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1684,20 +1684,20 @@ setLikeOptFlag(sqlite3 * db, const char *zName, u8 flagVal)
  * @retval none.
  */
 void
-sqlite3RegisterLikeFunctions(sqlite3 *db, int *is_case_insensitive)
+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 = SQLITE_PTR_TO_INT(is_case_insensitive);
+	int *is_like_ci = SQLITE_INT_TO_PTR(is_case_insensitive);
 	sqlite3CreateFunc(db, "LIKE", AFFINITY_INTEGER, 2, 0,
-			  is_case_insensitive, likeFunc, 0, 0, 0);
+			  is_like_ci, likeFunc, 0, 0, 0);
 	sqlite3CreateFunc(db, "LIKE", AFFINITY_INTEGER, 3, 0,
-			  is_case_insensitive, likeFunc, 0, 0, 0);
+			  is_like_ci, likeFunc, 0, 0, 0);
 	setLikeOptFlag(db, "LIKE",
-		       !(is_like_ci) ? (SQLITE_FUNC_LIKE |
+		       !(is_case_insensitive) ? (SQLITE_FUNC_LIKE |
 		       SQLITE_FUNC_CASE) : SQLITE_FUNC_LIKE);
 }
 
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 546b18ae2..8939aa702 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -586,10 +586,9 @@ 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,
-							     is_like_ci);
+				int is_like_ci =
+					!(sqlite3GetBoolean(zRight, 0));
+				sqlite3RegisterLikeFunctions(db, is_like_ci);
 			}
 			break;
 		}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index dd819622b..07e83e444 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -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 *,



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