Hello! Consider some comments please. Diff with previous version will be at the end. > On Sep 11, 2018, at 15:03, Alex Khatskevich 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( >> -- >> }) >> >> --- 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%' >> - ]], { >> - -- >> - 0 >> - -- >> - }) >> - >> -test:do_execsql_test( >> - "e_expr-17.1.2", >> - [[ >> - SELECT 'abcxyz' GLOB 'abc*' >> - ]], { >> - -- >> - 1 >> - -- >> - }) >> - >> -test:do_execsql_test( >> - "e_expr-17.1.3", >> - [[ >> - SELECT 'abcxyz' GLOB 'abc___' >> - ]], { >> - -- >> - 0 >> - -- >> - }) >> - >> -test:do_execsql_test( >> - "e_expr-17.1.4", >> - [[ >> - SELECT 'abcxyz' GLOB 'abc???' >> - ]], { >> - -- >> - 1 >> - -- >> - }) >> - >> -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%'; >> ]], { >> - -- >> + -- >> 1 >> - -- >> - }) >> - >> -test:do_execsql_test( >> - "e_expr-17.1.6", >> - [[ >> - SELECT 'ABCxyz' GLOB 'abc*' >> - ]], { >> - -- >> - 0 >> - -- >> - }) >> - >> -test:do_execsql_test( >> - "e_expr-17.1.7", >> - [[ >> - SELECT 'abcxyz' GLOB 'ABC*' >> - ]], { >> - -- >> - 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%' >> ]], { >> -- >> - 1 >> + 0 >> -- >> }) >> >> 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. >> -- >> - 0 >> - -- >> + >> + -- >> }) >> >> 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' >> - ]], { >> - -- >> - 1 >> - -- >> - }) >> - >> -test:do_test( >> - "e_expr-17.3.2", >> - function() >> - return globargs >> - end, { >> - -- >> - "def", "abc" >> - -- >> - }) >> - >> -globargs = { } >> -test:do_execsql_test( >> - "e_expr-17.3.3", >> - [[ >> - SELECT 'X' NOT GLOB 'Y' >> - ]], { >> - -- >> - 0 >> - -- >> - }) >> - >> -test:do_test( >> - "e_expr-17.3.4", >> - function() >> - return globargs >> - end, { >> - -- >> - "Y", "X" >> - -- >> - }) >> - >> --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( -- }) +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%' ]], { -- 0 @@ -2290,85 +2291,65 @@ test:do_execsql_test( test:do_execsql_test( "e_expr-17.2.2", - [[ - PRAGMA case_sensitive_like = 0 - ]], { - -- - - -- - }) - -test:do_execsql_test( - "e_expr-17.2.3", - [[ - SELECT 'abcxyz' NOT LIKE 'ABC%' - ]], { - -- - 0 - -- - }) - -test:do_execsql_test( - "e_expr-17.2.4", [[ SELECT 'abcxyz' NOT LIKE 'abc%' ]], { - -- + -- 0 - -- + -- }) test:do_execsql_test( - "e_expr-17.2.5", + "e_expr-17.2.3", [[ SELECT 'abdxyz' NOT LIKE 'abc%' ]], { - -- + -- 1 - -- + -- }) -- 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 ]], { - -- + -- "null" - -- + -- }) test:do_execsql_test( - "e_expr-17.2.7", + "e_expr-17.2.5", [[ SELECT 'abcxyz' NOT LIKE NULL ]], { - -- + -- "null" - -- + -- }) test:do_execsql_test( - "e_expr-17.2.8", + "e_expr-17.2.6", [[ SELECT NULL NOT GLOB 'abc*' ]], { - -- + -- "null" - -- + -- }) test:do_execsql_test( - "e_expr-17.2.9", + "e_expr-17.2.7", [[ SELECT NULL NOT LIKE 'ABC%' ]], { - -- + -- "null" - -- + -- }) db("nullvalue", "")