[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jul 22 22:17:16 MSK 2019
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 at 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.
More information about the Tarantool-patches
mailing list