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

[Vladislav Shpilevoy]

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.

On 18/07/2019 23:08, Vladislav Shpilevoy wrote:
> 
> 
> On 18/07/2019 22:56, n.pettik wrote:
>>
>>
>>> On 18 Jul 2019, at 23:18, Vladislav Shpilevoy <v.shpilevoy at tarantool.org <mailto:v.shpilevoy at tarantool.org>> wrote:
>>>
>>> Hi!
>>>
>>> Thanks for the fixes!
>>>
>>>>> -------------------------
>>>>> 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.
>