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

[Alexander Turenko <alexander.turenko@tarantool.org>]

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.

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

/** .. */ 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.

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

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

Indent is strange.

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

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

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

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

Trailing whitespaces.

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

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.