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

[Vladislav Shpilevoy]

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.

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

> ---
>  src/box/sql/func.c            | 38 ++++++++++++++++++++++++--------------
>  src/box/sql/sqlInt.h          |  3 ---
>  src/box/sql/vdbe.c            | 19 ++-----------------
>  src/box/sql/vdbeInt.h         |  3 +--
>  test/sql-tap/select1.test.lua |  4 ++--
>  test/sql-tap/select5.test.lua |  3 ++-
>  6 files changed, 31 insertions(+), 39 deletions(-)
>> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> 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.

>   *
>   * @param record Memory cell containing value to be converted.
>   * @retval 0 If value can be converted to integer or number.