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

["n.pettik" <korablev@tarantool.org>]

> On 19 Jul 2019, at 00:08, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> On 18/07/2019 22:56, n.pettik wrote:
>> 
>>> On 18 Jul 2019, at 23:18, Vladislav Shpilevoy <v.shpilevoy@tarantool.org <mailto:v.shpilevoy@tarantool.org>> wrote:
>>>>> -------------------------
>>>>> vdbe.c:307
>>>>> 
>>>>>> 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) {
>>>>>> int64_t i = (int64_t) record->u.r;
>>>>>> if (i == record->u.r)
>>>>>> mem_set_int(record, record->u.r,
>>>>>>     record->u.r <= -1);
>>>>>> return 0;
>>>>>> }
>>>>> 
>>>>> It is a part of function mem_apply_type. When target type is
>>>>> UNSIGNED, and a value is MEM_Int, you do nothing. Why? Looks like
>>>>> it is possible to pass here a negative value, and CAST UNSIGNED
>>>>> would do nothing.
>>>> 
>>>> Basically, this function implements sort of implicit cast
>>>> which takes place before comparison/assignment.
>>>> For comparisons it makes no sense - we can compare
>>>> integer with unsigned value - the latter is always greater.
>>>> For assignment it is also meaningless: if we attempt
>>>> at inserting negative values to unsigned field appropriate
>>>> error will be raised anyway. If you can come up with
>>>> specific example, let’s discuss it.
>>>> 
>>> 
>>> I can't provide a test. But the function is named mem_apply_type,
>>> and it doesn't apply type, when it is unsigned, and a value is
>>> negative. Doesn't it look wrong to you?
>>> 
>>> If some code wants to get an integer, it can apply FIELD_TYPE_INTEGER
>>> instead of FIELD_TYPE_UNSIGNED. IMO, an attempt to apply unsigned
>>> to int should raise an error here. Otherwise this function can't
>>> be named 'apply_type' because it ignores negative -> unsigned case.
>> 
>> Okay, let’s rename it. I can suggest these options:
>> 
>> mem_cast_implicit()
>> mem_cast_implicit_to_type()
>> mem_implicit_cast_to_type()
>> mem_convert_implicit()
>> mem_convert_to_type()
>> mem_type_coerce_implicit()
>> mem_type_implicit_coercion()
>> mem_type_coercion_implicit()
>> mem_implicit_type_juggling()
>> mem_implicit_juggle_to_type()
>> mem_do_implicit_conversion()
>> mem_do_implicit_coercion()
>> 
>> Or any other combination :)
>> 
> 
> But it is not implicit. It just does not work, when a value is negative,
> and type is unsigned.

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.