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

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

Hi!

I gave some minor comments and I think I cannot add any value anymore.
Nikita N., can you look into this patch?

> -               if (sqlite3_strglob("unordered*", z) == 0) {
> +               if (sql_strlike_cs("unordered%", z, '[') == 0)
>                         index->def->opts.stat->is_unordered = true;
> -               } else if (sqlite3_strglob("noskipscan*", z) == 0) {
> +               else if (sql_strlike_cs("noskipscan%", z, '[') == 0)
>                         index->def->opts.stat->skip_scan_enabled = false;
> -               }

I don't think '[' works here as it was in globs, so the parameter can be
just removed.

> -sqlite3_strglob(const char *zGlobPattern, const char *zString)
> +sql_strlike_cs(const char *zPattern, const char *zStr, unsigned int esc)

Why not UChar32, but 'unsigned int'? (It does not matter if the above is
true.)

See other comments below.

WBR, Alexander Turenko.

> > BTW, it seems that we add SQLITE_FUNC_CASE into flags, so maybe we can
> > read it in likeFunc and don't pass additional parameter at all? It is
> > just raw idea, I don't know the mechanics of sql functions much.
> 
> I’m not sure it’s a good idea, at least not in terms of this patch.
> Call to likeFunc comes right from OP_FUNCTION which is used for sql
> functions (of course it’s not only LIKE). Thus the list of parameters is
> pretty much defined there.
> 
> 	(*pCtx->pFunc->xSFunc)(pCtx, pCtx->argc, pCtx->argv);/* IMP: R-24505-23230 */
> 

I'll leave this question to consider for the next reviewer.

> >> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
> >> index eb4f43a90..dbbe1c0fe 100755
> >> --- a/test/sql-tap/collation.test.lua
> >> +++ b/test/sql-tap/collation.test.lua
> >> @@ -219,7 +219,7 @@ local like_testcases =
> >>         {0, {"Aab", "aaa"}} },
> >>     {"2.1.2",
> >>         "EXPLAIN QUERY PLAN SELECT * FROM tx1 WHERE s1 LIKE 'A%';",
> >> -        {0, {0, 0, 0, "/USING COVERING INDEX I1/"}} },
> >> +        {0, {0, 0, 0, "SEARCH TABLE TX1 USING COVERING INDEX I1 (S1>? AND S1<?)"}}},
> > 
> > What is this hunk about?
> 
> I found that this changed message occurs even on fresh 2.1.
> I was using this and few other test cases to test my patch so it was
> kind of concerned with the patch and i decided to fix it at the same time.
> (I don’t really think it does worth opening an issue).
> I assume this issue was caused by commit 6b8acd8fde and just
> wasn’t fixed.
> I guess I should’ve wrote about it in the previous letter.
> 

If so it should be separate commit. At least the message of this commit
should state the changes.