[tarantool-patches] Re: [PATCH 2/2] sql: remove GLOB from Tarantool
Nikita Tatunov
n.tatunov at tarantool.org
Thu Oct 18 23:28:12 MSK 2018
Hello! Consider some comments please.
Diff with previous version will be at the end.
> On Sep 11, 2018, at 15:03, Alex Khatskevich <avkhatskevich at 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", "")
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20181018/828491e6/attachment.html>
More information about the Tarantool-patches
mailing list