[tarantool-patches] Re: [PATCH 2/9] sql: disallow text values participate in sum() aggregate

[n.pettik]

> On 16 Apr 2019, at 17:12, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Hi! Thanks for the patch! See 3 comments below.
> 
> On 14/04/2019 18:04, Nikita Pettik wrote:
>> It is obvious that we can't add string with number except the case when
>> string is a number literal in quotes (aka '123' or '0.5'). Before this
>> patch values which can't be converted to numbers were just skipped.
>> Now error is raised. Another one misbehavior was in using
>> sql_value_numeric_type() function, which set flag indicating number
>> value in memory cell, but didn't clear MEM_Str flag.
> 
> 1. Before this concrete commit sql_value_numeric_type() worked correctly -
> it used your refactored mem_apply_numeric_type(), which clears the old
> type flags.

Ok, indeed. Fixed commit message to the next:

    sql: disallow text values participate in sum() aggregate
    
    It is obvious that we can't add string with number except the case when
    string is a number literal in quotes (aka '123' or '0.5'). Before this
    patch values which can't be converted to numbers were just skipped.
    Now error is raised. This patch also removes sql_value_numeric_type()
    function since it is not used anymore: instead we invoke
    mem_apply_numeric_type().


>> As a result, we
>> couldn't determine type of value in such memory cell without
>> ambigiousness.
> 
> 2. Can't find this word in a dictionary. Probably, ambiguousness?

Removed this mention at all. See above.

>> index 8cd00d43a..ec9123a66 100644
>> --- a/src/box/sql/vdbeInt.h
>> +++ b/src/box/sql/vdbeInt.h
>> @@ -274,8 +274,7 @@ mem_type_to_str(const struct Mem *p);
>>  * if we can do so without loss of information. Firstly, value
>>  * is attempted to be converted to integer, and in case of fail -
>>  * to floating point number. Note that function is assumed to be
>> - * called only on memory cell containing string,
>> - * i.e. memory->type == MEM_Str.
>> + * called on memory cell containing string, i.e. mem->type == MEM_Str.
> 
> 3. Please, move to the previous commit.

Moved.