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

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

Hello again, Alexander!
Here are some comments / fixes to the review.

Issues:
https://github.com/tarantool/tarantool/issues/3334
https://github.com/tarantool/tarantool/issues/3251
https://github.com/tarantool/tarantool/issues/3572

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

> On Oct 21, 2018, at 06:48, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> I don't have objections re this commit in general. Please, fix minor
> comments below and proceed with Nikita P.
> 
> WBR, Alexander Turenko.
> 
>> - * Providing there are symbols in string s this
>> - * macro returns UTF-8 code of character and
>> - * promotes pointer to the next symbol in the string.
>> - * Otherwise return code is SQL_END_OF_STRING.
>> + * Providing there are symbols in string s this macro returns
>> + * UTF-8 code of character and promotes pointer to the next
>> + * symbol in the string. If s points to an invalid UTF-8 symbol
>> + * return code is SQL_INVALID_UTF8_SYMBOL. If there're no symbols
>> + * left in string s return code is SQL_END_OF_STRING.
> 
>> -/*
>> - * Possible error returns from sql_utf8_pattern_compare()
>> +/**
>> + * Possible error returns from sql_utf8_pattern_compare().
>>  */
> 
>> - *      '%'       Matches any sequence of zero or more characters.
>> + *      '%'       Matches any sequence of zero or more
>> + *                characters.
> 
>> - *      Ec        Where E is the "esc" character and c is any other
>> - *                character, including '%', '_', and esc, match
>> - *                exactly c.
>> + *      Ec        Where E is the "esc" character and c is any
>> + *                other character, including '%', '_', and esc,
>> + *                match exactly c.
> 
>> -       const char * pattern_end = pattern + strlen(pattern);
>> -       const char * string_end = string + strlen(string);
>> +       const char *pattern_end = pattern + strlen(pattern);
>> +       const char *string_end = string + strlen(string);
> 
> Please, amend these hunks into the first patch. The first patch includes
> code style fixes, so it would look more appropriate in it, eps. when you
> already touch the code in it.

Done.

> 
>> +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);


> 
> /** .. */ inside functions should be /* */ (they are just comments and
> they aren't documentation comments). It appears here ans there, so,
> please, grep the intire patch for such things.

Fixed.

> 
>> -       return sql_utf8_pattern_compare(zGlobPattern, zString, &globInfo, '[');
>> +       return sql_utf8_pattern_compare(zPattern, zStr, case_sensitive_like, esc);
> 
>> -       return sql_utf8_pattern_compare(zPattern, zStr, &likeInfoNorm, esc);
>> +       return sql_utf8_pattern_compare(zPattern, zStr, case_insensitive_like, esc);
> 
> Exceeds 80 symbols

thank you, didn’t notice.

> 
>> + * @param is_case_sensitive whether like should be case sensitive
>> + *                      or not.
> 
> Indent is strange.

fixed.

> 
>> +       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.

> 
>> -int sqlite3IsLikeFunction(sqlite3 *, Expr *, int *, char *);
>> +int sql_is_like_func(sqlite3 *db, Expr *pExpr, int *is_case_insensitive);
> 
> The code around does not have parameter names, so here they are looks
> strange.

fixed.

> 
>> +       /* String on RHS of LIKE operator */
>> +       const char *z = 0;
>> +       /* Right and left size of LIKE operator */
>> +       Expr *pRight, *pLeft;
>> +       /* List of operands to the LIKE operator */
>> +       ExprList *pList;
>> +       /* One character in z[] */
>> +       int c;
>> +       /* Number of non-wildcard prefix characters */
>> +       int cnt;
>> +       /* Database connection */
>> +       sqlite3 *db = pParse->db;
>>        sqlite3_value *pVal = 0;
>> -       int op;                 /* Opcode of pRight */
>> -       int rc;                 /* Result code to return */
>> +       /* Opcode of pRight */
>> +       int op;
>> +       /* Result code to return */
>> +       int rc;
> 
> Our code style suggests to add a period at end of the comment. The same
> in exprAnalyze() below.

Thank you, fixed.

> 
>> +        INSERT INTO t1 SELECT a+5,6,'six'||substr(c,4) FROM t1 WHERE c LIKE 'one-%';
> 
> Trailing whitespaces.

deleted.

> 
>> --- NOTE: This test needs refactoring after deletion of GLOB &
>> ---      type restrictions for LIKE. (See #3572)
>> -- Another NOTE: MATCH & REGEXP aren't supported in Tarantool &
>> ---              are waiting for their hour, don't confuse them
>> ---              being commented with ticket above.
>> -    {"=", "==", "!=", "<>"}, --"LIKE", "GLOB"}, --"MATCH", "REGEXP"},
>> +--               are waiting for their hour, don't confuse them
>> +--               being commented with commenting of "LIKE".
>> +    {"=", "==", "!=", "<>"}, --"MATCH", "REGEXP"},
> 
> Another NOTE -> NOTE.
> 
> 'don't confuse them being commented with commenting of "LIKE"' -- can be
> removed.

Fixed.

> 
> test/sql-tap/like3.test.lua cases can be rewritten with LIKE instead of
> removing, right? I think it worth to do if it is so.

Ok, sure.