[tarantool-patches] Re: [PATCH] sql: LIKE & GLOB pattern comparison issue

[Alexander Turenko]

Minor comments are below. Looks good for me in general.

Please, review the patch with Lesha after the fixes.

WBR, Alexander Turenko.

> > > With the patch applied an error will be returned in case there's
> > > invalid UTF-8 symbol in pattern & pattern will not be matched
> > > with the string that contains it. Some minor corrections to function
> > > were made as well.
> > >
> >
> > Nitpicking: 'error will be returned' is third situatuon, other then
> > 'matched' and 'not matched'.
> >
> >
> I guess commit message is about changes. Do I really need to
> mention valid cases that didn't change?
> 

Ok, now I got that 'it' is 'invalid symbol'. Misunderstood it before as
description of the one case when a pattern has invalid UTF-8 symbols.
The sentence is correct. Maybe it can be clarified like so: '& correct
UTF-8 pattern will not be matched with a string with invalid UTF-8
symbols'. Or in another way you like.

'Some minor corrections to function were made as well' -- describe
certain behaviour or code changes (like 'fixed infinite loop during
pattern matching and incorrect matching as well') or remove this
abstract sentence.

> > The function itself looks good except lines longer then 80 columns.
> >
> 
> As i said before I don't think breaking these lines will increase
> readability.
> 

There are two little length exceed place in the code, it needs some
refactoring to decrease code blocks nesting level. I think we can lease
it as is for now (if other reviewer don't ask for that).

But comments within the function are worth to fix to fit the limit.

> +test:do_catchsql_set_test(like_test_cases, prefix)
> +
> +-- Invalid testcases.
> +
> +for i, tested_string in ipairs(invalid_testcases) do
> +
> +-- We should raise an error if pattern contains invalid characters.

Again, indent comments to the same level as code in the block.

If a comment is related to several code blocks (with empty lines btw),
then separate it with the empty line from the code (to don't
misinterpret it as related to the first block).

> + local test_name = prefix .. "2." .. tostring(i)
> + local test_itself = "SELECT 'abc' LIKE 'ab" .. tested_string .. "';"
> + test:do_catchsql_test(
> + test_name,
> + test_itself, {
> + -- <test_name>
> + 1, "LIKE or GLOB pattern can only contain UTF-8 characters"
> + -- <test_name>
> + })
> +

Are there reason to format it like so? Maybe just:

test:do_catchsql_test(test_name, test_itself,
    {1, "LIKE or GLOB pattern can only contain UTF-8 characters"})