[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi!

TLDR: LGTM.

Detailed answer below.

On 22/07/2019 12:20, n.pettik wrote:
> 
> 
>> On 19 Jul 2019, at 00:13, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Why are you keep trying not to fix this obvious bug?
>> If that path is unreachable, then either delete it,
>> or add an assertion. Or fix this place.
> 
> Sorry, message threads now seem to be entangled a bit,
> so I guess you are talking about valueFromExpr() func.
> I’ve removed the single reachable to this function path,
> but left function itself (valueFromExpr()) since it seem
> to be quite meaningful and can be applied somewhere
> later. In the next patch I’ve added assertion fault to the
> place of conversion of double negative values (-(-5)):

No, I was talking about mem_apply_type. With the place you
are talking above I agree.

Sorry, I will bring your answer from another email here:

> So, you want to see smth like this:
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 73a1f321b..835544d44 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -306,8 +306,6 @@ mem_apply_type(struct Mem *record, enum field_type type)
>         switch (type) {
>         case FIELD_TYPE_INTEGER:
>         case FIELD_TYPE_UNSIGNED:
> -               if ((record->flags & MEM_Int) == MEM_Int)
> -                       return 0;
>                 if ((record->flags & MEM_UInt) == MEM_UInt)
>                         return 0;
>                 if ((record->flags & MEM_Real) == MEM_Real) {
> @@ -317,7 +315,14 @@ mem_apply_type(struct Mem *record, enum field_type type)
>                                             record->u.r <= -1);
>                         return 0;
>                 }
> -               return sqlVdbeMemIntegerify(record, false);
> +               if (sqlVdbeMemIntegerify(record, false) != 0)
> +                       return -1;
> +               if ((record->flags & MEM_Int) == MEM_Int) {
> +                       if (type == FIELD_TYPE_UNSIGNED)
> +                               return -1;
> +                       return 0;
> +               }
> +               return 0;
> 
> The difference can be seen in queries like this:
> 
> box.execute("CREATE TABLE t1 (id UNSIGNED PRIMARY KEY);")
> box.execute("INSERT INTO t1 VALUES (-3);")
> ---
> - error: 'Type mismatch: can not convert -3 to unsigned'
> …
> 
> Without this change we got:
> 'Tuple field 1 type does not match one required by operation: expected unsigned’
> 
> I consider this check to be a bit inappropriate since It is redundant.
> Comparison between unsigned and signed is well defined;
> insertion to the unsigned field is prohibited and checked in more
> low level core mechanisms. I would say I don’t mind applying this
> change (and already applied to speed up review process), but on
> the other side I don’t understand why do we need to add extra checks
> on top (SQL) layer.

I agree, all the checks should be done by box when possible. The only
single problem I am talking about is not a check, but a function name
mismatching what it is doing. 'mem_apply_type' does not apply unsigned
type to a negative value. I just feel uncomfortable about that. The
function just ignores this case.

I was thinking, that perhaps some day the function mem_apply_type will
evolve and consume MemCast and other functions related to type conversions,
and we will forget to fix this place, and will got a bug.

At this moment it is not a bug, because mem_apply_type is used only
before calling comparators, which check types in box. But if someday it
will be used for something else, we will forget that it ignores negative
-> unsigned conversion.

Anyway, never mind. The patchset LGTM.