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

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

> On Oct 29, 2018, at 16:01, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> The patch is okay except one note re test case.
> 
> WBR, Alexander Turenko.
> 
>> --- 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.
> 
> This test case was removed, while we have not a similar one for LIKE.

I guess it is concerned more with the second patch.
Anyways. If you mean the tests following this comment then
actually there are some similar tests for LIKE (15.1.x).

> 
>>>>>   if(((size_t)(sourceLimit-s)>(size_t)0x7fffffff && sourceLimit>s)) {
>>>>>       *err=U_ILLEGAL_ARGUMENT_ERROR;
>>>>>       return 0xffff;
>>>> 
>>>> 4. I’m not sure if string data can be this long in our context. 
>>>>  (string length > (size_t) 0x7ffffffff)
>>> 
>>> Note: not 0x7ffffffff, but 0x7fffffff.
>>> 
>>> This limit seems to be some weird internal thing related to using
>>> ucnv_getNextUChar inside libicu.
>>> 
>>> I propose to lie libicu about the buffer size in case when it exceeds
>>> this limit. A UTF-8 encoded symbol is 4 bytes long at max, so we can
>>> pass the following instead of pattern_end:
>>> 
>>> ((size_t) (pattern_end - pattern) > (size_t) 0x7fffffff ? pattern + 0x7fffffff : pattern_end
>>> 
>>> I think this trick need to be covered with a unit test (because it is unclear
>>> how to create a string of size >1GiB from lua). Don't sure whether it is
>>> okay to allocate such amount of memory in the test, though...
>>> 
>>> Please, don't do that within this patch, because it is about the another bug.
>>> File an issue with all needed information instead (you can provide a link to
>>> this message for example).
>> 
>> Ok, thank you for advice. I think that’s a good idea, but there’s one thing
>> I’m getting concerned about: it will cause a lot of operations especially
>> in case we’re using LIKE for scanning a lot of data (). I guess even if it’s
>> relevant it’s a discussion inside of an issue that’s going to be filed.
> 
> Filed https://github.com/tarantool/tarantool/issues/3773
> 
>>>>>           } else if(U_SUCCESS(*err) && c>=0) {
>>>>>               return c;
>>>> 
>>>> 6. Returns symbol (can also be 0xfffd, as it is not treated as an actual error).
>>>> 
>>>> So if I’m not mistaken we will get results in our function either from
>>>> ‘return’ number 5 or number 6 and the following code will not be executed.
>>> 
>>> It is not so. We'll fall through in case of U_ILLEGAL_CHAR_FOUND or
>>> U_TRUNCATED_CHAR_FOUND error.
>>> 
>>> To be honest I don't want to continue. It seems we should not lean on
>>> the fact that 0xffff always means end of the buffer, because it does not
>>> guaranteed by the API and is not clear from the code.
>>> 
>>> AFAIR, the problem was to choose appropriate symbol to mark end of the
>>> buffer situation and distinguish it from a real error. It seems we have
>>> not one. So we should fairly (and always) check for the buffer before a
>>> call to ucnv_getNextUChar() or check the status it provide after the
>>> call. I would prefer to check it in our code. It seems that it is how
>>> the API works.
>>> 
>>> I propose to use the same code pattern for all Utf8Read calls, e.g.:
>>> 
>>> if (pattern < pattern_end)
>>> 	c = Utf8Read(pattern, pattern_end)
>>> else
>>> 	return SQL_...;
>>> if (c == SQL_INVALID_UTF8_SYMBOL)
>>> 	return SQL_...;
>>> assert(U_SUCCESS(status));
>>> 
>>> Note: I have added the assert, because it is not clear what we can do
>>> with, say, U_INVALID_TABLE_FORMAT (improper libicu build /
>>> installation). Hope Nikita P. suggests right way, but now I think we
>>> should at least assert on that.
>>> 
>>> It seems the code above can be even wrapped into a macro that will get
>>> two pointers (pattern and pattern_end / string and string_end) and two
>>> SQL_...  error code to handle two possible errors. Yep, it is generally
>>> discouraged to return from a macro, but if it'll greatly improves the
>>> code readability, so it is appropriate, I think. Just define the macro
>>> right before the function and undefne it after to show a reader it is
>>> some pure internal thing.
>>> 
>>> Note: If you will going that way, don't wrap Utf8Read macro into another
>>> macro. Use one with ucnv_getNextUChar call.
>>> 
>>> It is refactoring of the code and our of the scope of your issue.
>>> Please, file an issue and link this message into it (but please ask
>>> Nikita P. opinion before).
>>> 
>>> It is not good IMHO, but it seems now it worth to leave the code with
>>> assumption 0xffff is the end of buffer. This is kind of splitting the
>>> problem into parts and allow us to proceed with this patch re parsing
>>> bug.
> 
> Filed https://github.com/tarantool/tarantool/issues/3774