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

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

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

Hello! Consider some comments please.
Diff with previous version will be at the end.

> On Sep 11, 2018, at 15:03, Alex Khatskevich <avkhatskevich@tarantool.org> wrote:
> 
> 
>>>>>> 
>>>>>> -					     "ESCAPE expression must be a single character",
>>>>>> +					     "ESCAPE expression must be a"
>>>>>> +					     " single character",
>>>>> Do not split error messages at the middle of a sentence. It makes errors ungreppable.
>>>>> Make it <80 somehow different.
>>>>> 
>>>> 
>>>> Have already been discussed in this thread.
>>> I suppose that we concluded to try to fit into 80 and split the string only
>>> in case it is impossible.
>> 
>> I don’t think so. Anyways, Alexander could you please give your thoughts?
> Discussed with Nikita, Alexander, Vladimir, Kirill... Conclusion: use `const char temp variable` if possible.
> like this:
> ``` 
>         const char *const err_msg = 
>             "ESCAPE expression must be a single character"; 
>         if (sqlite3Utf8CharLen((char *)zEsc, -1) != 1) { 
>             sqlite3_result_error(context, 
>                          err_msg, 
>                          -1); 
>             return; 
> ``` 
> 
> If it does not help (but it is) split the message.

Splitted both error messages.

>>>> diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
>>>> index 162026845..0d69e8535 100755
>>>> --- a/test/sql-tap/e_expr.test.lua
>>>> +++ b/test/sql-tap/e_expr.test.lua
>>>> @@ -1,6 +1,6 @@
>>>>  #!/usr/bin/env tarantool
>>>>  test = require("sqltester")
>>>> -test:plan(11521)
>>>> +test:plan(10647)
>>>>  
>>>>  --!./tcltestrunner.lua
>>>>  -- 2010 July 16
>>>> @@ -77,7 +77,7 @@ local operations = {
>>>>      {"<>", "ne1"},
>>>>      {"!=", "ne2"},
>>>>      {"IS", "is"},
>>>> -    {"LIKE", "like"},
>>>> +--    {"LIKE", "like"},
>>>>      {"AND", "and"},
>>>>      {"OR", "or"},
>>>>      {"MATCH", "match"},
>>>> @@ -96,8 +96,9 @@ operations = {
>>>>      {"<<", ">>", "&", "|"},
>>>>      {"<", "<=", ">", ">="},
>>>>  -- Another NOTE: MATCH & REGEXP aren't supported in Tarantool &
>>>> --- 		 are waiting for their hour.
>>>> -    {"=", "==", "!=", "<>", "LIKE"}, --"MATCH", "REGEXP"},
>>>> +--               are waiting for their hour, don't confuse them
>>>> +--               being commented with commenting of "LIKE".
>>>> +    {"=", "==", "!=", "<>"}, --"LIKE"}, --"MATCH", "REGEXP"},
>>> Delete Like. No one returns here.
>> 
>> It’s a table of all of the operators thus I think it still worth leaving it there.
>> Who knows, it may be that someone will revive tests for LIKE.
> Delete Like. No one returns here.
> We do not need a garbage. Like is not relevant anymore for this test.

Deleted.

>> 
>>  /*
>> - * Maximum length (in bytes) of the pattern in a LIKE or GLOB
>> - * operator.
>> + * Maximum length (in bytes) of the pattern in a LIKE operator.
>>   */
>>  #ifndef SQLITE_MAX_LIKE_PATTERN_LENGTH
>>  #define SQLITE_MAX_LIKE_PATTERN_LENGTH 50000
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index 0c978142d..3f10f4d68 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -5521,7 +5521,7 @@ vdbe_return:
>>  	testcase( nVmStep>0);
>>  	p->aCounter[SQLITE_STMTSTATUS_VM_STEP] += (int)nVmStep;
>>  	assert(rc!=SQLITE_OK || nExtraDelete==0
>> -		|| sqlite3_strlike("DELETE%",p->zSql,0)!=0
>> +		|| sql_strlike_ci("DELETE%", p->zSql, 0) != 0
>>  		);
>>  	return rc;
>>  
>> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
>> index c35c25ac4..f864ea7fa 100644
>> --- a/src/box/sql/wherecode.c
>> +++ b/src/box/sql/wherecode.c
>> @@ -339,7 +339,7 @@ sqlite3WhereAddScanStatus(Vdbe * v,		/* Vdbe to add scanstatus entry to */
>>   * automatically disabled.  In this way, terms get disabled if derived
>>   * virtual terms are tested first.  For example:
>>   *
>> - *      x GLOB 'abc*' AND x>='abc' AND x<'acd'
>> + *      x LIKE 'abc%' AND x>='abc' AND x<'acd'
>>   *      \___________/     \______/     \_____/
>>   *         parent          child1       child2
>>   *
>> diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
>> index 612868695..2d9fb6453 100644
>> --- a/src/box/sql/whereexpr.c
>> +++ b/src/box/sql/whereexpr.c
>> @@ -218,38 +218,61 @@ operatorMask(int op)
>>  	return c;
>>  }
>>  
>> +/**
>> + * Wildcard characters.
>> + */
>> +#define match_one '_'
>> +#define match_all '%'
> If possible - move the define to a header which both (whereexpr and func) file include.
> This also require to give more descriptive name, e.g. LIKE_MATCH_ONE.

Moved them to sqliteInt.h and renamed to:
    1) `MATCH_ALL_WILDCARD`.
    2) `MATCH_ONE_WILDCARD`.
 
>>  for _, val in ipairs(test_cases12) do
>> @@ -1802,7 +1792,7 @@ test:do_execsql_test(
>>      })
>>  
>>  ---------------------------------------------------------------------------
>> --- Test the statements related to the LIKE and GLOB operators.
>> +-- Test the statements related to the LIKE operator.
>>  --
>>  -- EVIDENCE-OF: R-16584-60189 The LIKE operator does a pattern matching
>>  -- comparison.
>> @@ -2274,102 +2264,38 @@ test:do_execsql_test(
>>          -- </e_expr-16.1.7>
>>      })
>>  
>> --- EVIDENCE-OF: R-52087-12043 The GLOB operator is similar to LIKE but
>> --- uses the Unix file globbing syntax for its wildcards.
>> ---
>> --- EVIDENCE-OF: R-09813-17279 Also, GLOB is case sensitive, unlike LIKE.
>> +-- EVIDENCE-OF: R-39616-20555 LIKE may be preceded by the
>> +-- NOT keyword to invert the sense of the test.
>>  --
>>  test:do_execsql_test(
>> -    "e_expr-17.1.1",
>> -    [[
>> -        SELECT 'abcxyz' GLOB 'abc%'
>> -    ]], {
>> -        -- <e_expr-17.1.1>
>> -        0
>> -        -- </e_expr-17.1.1>
>> -    })
>> -
>> -test:do_execsql_test(
>> -    "e_expr-17.1.2",
>> -    [[
>> -        SELECT 'abcxyz' GLOB 'abc*'
>> -    ]], {
>> -        -- <e_expr-17.1.2>
>> -        1
>> -        -- </e_expr-17.1.2>
>> -    })
>> -
>> -test:do_execsql_test(
>> -    "e_expr-17.1.3",
>> -    [[
>> -        SELECT 'abcxyz' GLOB 'abc___'
>> -    ]], {
>> -        -- <e_expr-17.1.3>
>> -        0
>> -        -- </e_expr-17.1.3>
>> -    })
>> -
>> -test:do_execsql_test(
>> -    "e_expr-17.1.4",
>> -    [[
>> -        SELECT 'abcxyz' GLOB 'abc???'
>> -    ]], {
>> -        -- <e_expr-17.1.4>
>> -        1
>> -        -- </e_expr-17.1.4>
>> -    })
>> -
>> -test:do_execsql_test(
>> -    "e_expr-17.1.5",
>> +    "e_expr-17.2.0",
>>      [[
>> -        SELECT 'abcxyz' GLOB 'abc*'
>> +        PRAGMA case_sensitive_like = 1;
>> +        SELECT 'abcxyz' NOT LIKE 'ABC%';
>>      ]], {
>> -        -- <e_expr-17.1.5>
>> +        -- <e_expr-17.2.0>
>>          1
>> -        -- </e_expr-17.1.5>
>> -    })
>> -
>> -test:do_execsql_test(
>> -    "e_expr-17.1.6",
>> -    [[
>> -        SELECT 'ABCxyz' GLOB 'abc*'
>> -    ]], {
>> -        -- <e_expr-17.1.6>
>> -        0
>> -        -- </e_expr-17.1.6>
>> -    })
>> -
>> -test:do_execsql_test(
>> -    "e_expr-17.1.7",
>> -    [[
>> -        SELECT 'abcxyz' GLOB 'ABC*'
>> -    ]], {
>> -        -- <e_expr-17.1.7>
>> -        0
>> -        -- </e_expr-17.1.7>
>> +        -- </e_expr-17.2.0>
>>      })
>>  
>> --- EVIDENCE-OF: R-39616-20555 Both GLOB and LIKE may be preceded by the
>> --- NOT keyword to invert the sense of the test.
>> ---
>>  test:do_execsql_test(
>>      "e_expr-17.2.1",
>>      [[
>> -        SELECT 'abcxyz' NOT GLOB 'ABC*'
>> +        SELECT 'abcxyz' NOT LIKE 'abc%'
>>      ]], {
>>          -- <e_expr-17.2.1>
>> -        1
>> +        0
>>          -- </e_expr-17.2.1>
>>      })
>>  
>>  test:do_execsql_test(
>>      "e_expr-17.2.2",
>>      [[
>> -        SELECT 'abcxyz' NOT GLOB 'abc*'
>> +        PRAGMA case_sensitive_like = 0
>>      ]], {
> delete 17.2.1 17.2.2 (it was creaget for glob, like tested below), and move 
> `PRAGMA case_sensitive_like = 0` out of the test (it is not a test) (use just test:execsql or box.exequte)

Done.

>>          -- <e_expr-17.2.2>
>> -        0
>> -        -- </e_expr-17.2.2>
>> +
>> +        -- <e_expr-17.2.2>
>>      })
>>  
>>  test:do_execsql_test(
>> @@ -2448,62 +2374,6 @@ if 0>0 then
>>      db("nullvalue", "")
>>  end
>>  
>> --- EVIDENCE-OF: R-39414-35489 The infix GLOB operator is implemented by
>> --- calling the function glob(Y,X) and can be modified by overriding that
>> --- function.
>> -
>> -local globargs = {}
>> -local function globfunc(...)
>> -    local args = {...}
>> -    for i, v in ipairs(args) do
>> -        table.insert(globargs, v)
>> -    end
>> -    return 1
>> -end
>> -box.internal.sql_create_function("GLOB", globfunc, 2)
>> ---db("func", "glob", "-argcount", 2, "globfunc")
> Do not delete this test. Rewrite it for like.

There’re literally almost the same tests for like (15.1.x).

>> -
>> -test:do_execsql_test(
>> -    "e_expr-17.3.1",
>> -    [[
>> -        SELECT 'abc' GLOB 'def'
>> -    ]], {
>> -        -- <e_expr-17.3.1>
>> -        1
>> -        -- </e_expr-17.3.1>
>> -    })
>> -
>> -test:do_test(
>> -    "e_expr-17.3.2",
>> -    function()
>> -        return globargs
>> -    end, {
>> -        -- <e_expr-17.3.2>
>> -        "def", "abc"
>> -        -- </e_expr-17.3.2>
>> -    })
>> -
>> -globargs = {  }
>> -test:do_execsql_test(
>> -    "e_expr-17.3.3",
>> -    [[
>> -        SELECT 'X' NOT GLOB 'Y'
>> -    ]], {
>> -        -- <e_expr-17.3.3>
>> -        0
>> -        -- </e_expr-17.3.3>
>> -    })
>> -
>> -test:do_test(
>> -    "e_expr-17.3.4",
>> -    function()
>> -        return globargs
>> -    end, {
>> -        -- <e_expr-17.3.4>
>> -        "Y", "X"
>> -        -- </e_expr-17.3.4>
>> -    })
>> -
>>  --sqlite3("db", "test.db")
>>  -- EVIDENCE-OF: R-41650-20872 No regexp() user function is defined by
>>  -- default and so use of the REGEXP operator will normally result in an
>> diff --git a/test/sql-tap/gh-3251-string-pattern-comparison.test.lua b/test/sql-tap/gh-3251-string-pattern-comparison.test.lua
>> index addf0e36d..a6d822ccd 100755

diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 28b435ae3..f57967bd8 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -631,12 +631,6 @@ static const int case_insensitive_like = 1;
  */
 static const int case_sensitive_like = 0;
 
-/**
- * Wildcards.
- */
-#define match_one '_'
-#define match_all '%'
-
 /**
  * Possible error returns from sql_utf8_pattern_compare().
  */
@@ -693,7 +687,7 @@ sql_utf8_pattern_compare(const char *pattern,
 		c = Utf8Read(pattern, pattern_end);
 		if (c == SQL_INVALID_UTF8_SYMBOL)
 			return SQL_INVALID_PATTERN;
-		if (c == match_all) {
+		if (c == MATCH_ALL_WILDCARD) {
 			/**
 			 * Skip over multiple "%" characters in
 			 * the pattern. If there are also "_"
@@ -705,9 +699,10 @@ sql_utf8_pattern_compare(const char *pattern,
 			       SQL_END_OF_STRING) {
 				if (c == SQL_INVALID_UTF8_SYMBOL)
 					return SQL_INVALID_PATTERN;
-				if (c != match_all && c != match_one)
+				if (c != MATCH_ALL_WILDCARD &&
+				    c != MATCH_ONE_WILDCARD)
 					break;
-				if (c == match_one &&
+				if (c == MATCH_ONE_WILDCARD &&
 				    (c2 = Utf8Read(string, string_end)) ==
 				    SQL_END_OF_STRING)
 					return SQL_NOWILDCARDMATCH;
@@ -801,7 +796,7 @@ sql_utf8_pattern_compare(const char *pattern,
 			    c == u_tolower(c2))
 				continue;
 		}
-		if (c == match_one && pattern != zEscaped &&
+		if (c == MATCH_ONE_WILDCARD && pattern != zEscaped &&
 		    c2 != SQL_END_OF_STRING)
 			continue;
 		return SQL_NOMATCH;
@@ -894,11 +889,10 @@ likeFunc(sqlite3_context *context, int argc, sqlite3_value **argv)
 		const unsigned char *zEsc = sqlite3_value_text(argv[2]);
 		if (zEsc == 0)
 			return;
+		const char *const err_msg =
+			"ESCAPE expression must be a single character";
 		if (sqlite3Utf8CharLen((char *)zEsc, -1) != 1) {
-			sqlite3_result_error(context,
-					     "ESCAPE expression must be a"
-					     " single character",
-					     -1);
+			sqlite3_result_error(context, err_msg, -1);
 			return;
 		}
 		escape = sqlite3Utf8Read(&zEsc);
@@ -911,8 +905,9 @@ likeFunc(sqlite3_context *context, int argc, sqlite3_value **argv)
 	int res;
 	res = sql_utf8_pattern_compare(zB, zA, *is_like_ci, escape);
 	if (res == SQL_INVALID_PATTERN) {
-		sqlite3_result_error(context, "LIKE pattern can only contain"
-				     " UTF-8 characters", -1);
+		const char *const err_msg =
+			"LIKE pattern can only contain UTF-8 characters";
+		sqlite3_result_error(context, err_msg, -1);
 		return;
 	}
 	sqlite3_result_int(context, res == SQL_MATCH);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index a805adf22..420c76bfe 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -564,6 +564,12 @@ sqlite3_snprintf(int, char *, const char *, ...);
 char *
 sqlite3_vsnprintf(int, char *, const char *, va_list);
 
+/**
+ * Wildcard characters used in REGEXP-like operators.
+ */
+#define MATCH_ONE_WILDCARD '_'
+#define MATCH_ALL_WILDCARD '%'
+
 int
 sql_strlike_cs(const char *zLike, const char *zStr, unsigned int cEsc);
 
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 2d9fb6453..616acc640 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -218,12 +218,6 @@ operatorMask(int op)
 	return c;
 }
 
-/**
- * Wildcard characters.
- */
-#define match_one '_'
-#define match_all '%'
-
 #ifndef SQLITE_OMIT_LIKE_OPTIMIZATION
 /**
  * Check to see if the given expression is a LIKE operator that
@@ -305,11 +299,13 @@ is_like(Parse *pParse,
 	}
 	if (z) {
 		cnt = 0;
-		while ((c = z[cnt]) != 0 && c != match_one && c != match_all)
+		while ((c = z[cnt]) != 0 && c != MATCH_ONE_WILDCARD &&
+		       c != MATCH_ALL_WILDCARD)
 			cnt++;
 		if (cnt != 0 && 255 != (u8) z[cnt - 1]) {
 			Expr *pPrefix;
-			*pisComplete = c == match_all && z[cnt + 1] == 0;
+			*pisComplete = c == MATCH_ALL_WILDCARD &&
+				       z[cnt + 1] == 0;
 			pPrefix = sqlite3Expr(db, TK_STRING, z);
 			if (pPrefix)
 				pPrefix->u.zToken[cnt] = 0;
diff --git a/test/sql-tap/e_expr.test.lua b/test/sql-tap/e_expr.test.lua
index 0d69e8535..6d1edcfd0 100755
--- a/test/sql-tap/e_expr.test.lua
+++ b/test/sql-tap/e_expr.test.lua
@@ -77,7 +77,6 @@ local operations = {
     {"<>", "ne1"},
     {"!=", "ne2"},
     {"IS", "is"},
---    {"LIKE", "like"},
     {"AND", "and"},
     {"OR", "or"},
     {"MATCH", "match"},
@@ -98,7 +97,7 @@ operations = {
 -- Another NOTE: MATCH & REGEXP aren't supported in Tarantool &
 --               are waiting for their hour, don't confuse them
 --               being commented with commenting of "LIKE".
-    {"=", "==", "!=", "<>"}, --"LIKE"}, --"MATCH", "REGEXP"},
+    {"=", "==", "!=", "<>"}, --"MATCH", "REGEXP"},
     {"AND"},
     {"OR"},
 }
@@ -2278,10 +2277,12 @@ test:do_execsql_test(
         -- </e_expr-17.2.0>
     })
 
+test:execsql("PRAGMA case_sensitive_like = 0;")
+
 test:do_execsql_test(
     "e_expr-17.2.1",
     [[
-        SELECT 'abcxyz' NOT LIKE 'abc%'
+        SELECT 'abcxyz' NOT LIKE 'ABC%'
     ]], {
         -- <e_expr-17.2.1>
         0
@@ -2290,85 +2291,65 @@ test:do_execsql_test(
 
 test:do_execsql_test(
     "e_expr-17.2.2",
-    [[
-        PRAGMA case_sensitive_like = 0
-    ]], {
-        -- <e_expr-17.2.2>
-
-        -- <e_expr-17.2.2>
-    })
-
-test:do_execsql_test(
-    "e_expr-17.2.3",
-    [[
-        SELECT 'abcxyz' NOT LIKE 'ABC%'
-    ]], {
-        -- <e_expr-17.2.3>
-        0
-        -- </e_expr-17.2.3>
-    })
-
-test:do_execsql_test(
-    "e_expr-17.2.4",
     [[
         SELECT 'abcxyz' NOT LIKE 'abc%'
     ]], {
-        -- <e_expr-17.2.4>
+        -- <e_expr-17.2.2>
         0
-        -- </e_expr-17.2.4>
+        -- </e_expr-17.2.2>
     })
 
 test:do_execsql_test(
-    "e_expr-17.2.5",
+    "e_expr-17.2.3",
     [[
         SELECT 'abdxyz' NOT LIKE 'abc%'
     ]], {
-        -- <e_expr-17.2.5>
+        -- <e_expr-17.2.3>
         1
-        -- </e_expr-17.2.5>
+        -- </e_expr-17.2.3>
     })
 
 -- MUST_WORK_TEST uses access to nullvalue... (sql parameters) and built in functions
 if 0>0 then
     db("nullvalue", "null")
     test:do_execsql_test(
-        "e_expr-17.2.6",
+        "e_expr-17.2.4",
         [[
             SELECT 'abcxyz' NOT GLOB NULL
         ]], {
-            -- <e_expr-17.2.6>
+            -- <e_expr-17.2.4>
             "null"
-            -- </e_expr-17.2.6>
+            -- </e_expr-17.2.4>
         })
 
     test:do_execsql_test(
-        "e_expr-17.2.7",
+        "e_expr-17.2.5",
         [[
             SELECT 'abcxyz' NOT LIKE NULL
         ]], {
-            -- <e_expr-17.2.7>
+            -- <e_expr-17.2.5>
             "null"
-            -- </e_expr-17.2.7>
+            -- </e_expr-17.2.5>
         })
 
     test:do_execsql_test(
-        "e_expr-17.2.8",
+        "e_expr-17.2.6",
         [[
             SELECT NULL NOT GLOB 'abc*'
         ]], {
-            -- <e_expr-17.2.8>
+            -- <e_expr-17.2.6>
             "null"
-            -- </e_expr-17.2.8>
+            -- </e_expr-17.2.6>
         })
 
     test:do_execsql_test(
-        "e_expr-17.2.9",
+        "e_expr-17.2.7",
         [[
             SELECT NULL NOT LIKE 'ABC%'
         ]], {
-            -- <e_expr-17.2.9>
+            -- <e_expr-17.2.7>
             "null"
-            -- </e_expr-17.2.9>
+            -- </e_expr-17.2.7>
         })
 
     db("nullvalue", "")


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