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

[Vladislav Shpilevoy]

Thanks for the fixes!

On 17/07/2019 02:54, n.pettik wrote:
> 
>>>> --------------------------------------------------------------
>>>> vdbeaux.c:2998:
>>>> 		if ((f1 & MEM_UInt) != 0) {
>>>> 			if ((f2 & MEM_Real) != 0) {
>>>> 				return sqlIntFloatCompare(pMem1->u.i,
>>>>
>>>> pMem1 is unsigned, according to the first check,
>>>> but you use u.i. Why?
>>>
>>> Thx, I’ve fixed series of similar places and extended sql/types.test.lua:
>>>
>>> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
>>> index 325c54c18..b6b5cd0bf 100644
>>> --- a/src/box/sql/vdbeaux.c
>>> +++ b/src/box/sql/vdbeaux.c
>>> @@ -2887,43 +2887,50 @@ sqlBlobCompare(const Mem * pB1, const Mem * pB2)
>>>        return n1 - n2;
>>> }
>>>
>>> -/*
>>> - * Do a comparison between a 64-bit signed integer and a 64-bit floating-point
>>> - * number.  Return negative, zero, or positive if the first (i64) is less than,
>>> - * equal to, or greater than the second (double).
>>> +/**
>>> + * Do a comparison between a 64-bit unsigned/signed integer and a
>>> + * 64-bit floating-point number.  Return negative, zero, or
>>> + * positive if the first (integer) is less than, equal to, or
>>> + * greater than the second (double).
>>>  */
>>> static int
>>> -sqlIntFloatCompare(i64 i, double r)
>>> +compare_uint_float(uint64_t u, double r)
>>
>> Unfortunately, it is not as simple as you implemented it.
>> See mp_compare_double_uint64 in tuple_compare.cc for details.
>> In short, your function works wrong when numbers are near
>> uint maximum. Perhaps, it is worth moving this comparator
>> from tuple_compare.cc to a header. Like trivia/util.h.
> 
> Yep, I’m realising that :)
> Unfortunately, sqlite uses such simplified version and there
> are tests verifying this behaviour (boundary1/2/3 test suits).
> Patch fixing it (as you suggest) is trivial, but it requires monotonous
> test changes. Don’t you mind if I send it as follow-up after this
> patch-set hits the trunk?

It is ok. Please, create an issue.

>>>> --------------------------------------------------------------
>>>> vdbemem.c:1431:
>>> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
>>> index f8673912e..64acb5d41 100644
>>> --- a/src/box/sql/vdbemem.c
>>> +++ b/src/box/sql/vdbemem.c
>>> @@ -1428,11 +1428,15 @@ valueFromExpr(sql * db, /* The database connection */
>>>                                return rc;
>>>                        if (pVal->flags & MEM_Real) {
>>>                                pVal->u.r = -pVal->u.r;
>>> -                       } else if (pVal->u.i == SMALLEST_INT64) {
>>> -                               pVal->u.r = -(double)SMALLEST_INT64;
>>> -                               MemSetTypeFlag(pVal, MEM_Real);
>>> -                       } else {
>>> -                               pVal->u.i = -pVal->u.i;
>>> +                       } else if ((pVal->flags & MEM_Int) != 0) {
>>> +                               mem_set_u64(pVal, (uint64_t)(-pVal->u.i));
>>> +                       } else if ((pVal->flags & MEM_UInt) != 0) {
>>> +                               if (pVal->u.u > (uint64_t) INT64_MAX + 1) {
>>> +                                       pVal->u.r = -(double) pVal->u.u;
>>> +                                       MemSetTypeFlag(pVal, MEM_Real);
>>
>> Won't we have a problem here, that an expression '--value' won't be
>> equal to 'value' in case the value is bigger than INT64_MAX + 1?
> 
> I guess it’s OK according to the original code. What is more, I doubt
> that this path is reachable at all: both select -(-18446744073709551615)
> and select -(18446744073709551615) queries result in error while
> processing expr_code_int()..
> 

Then why do we keep that code, if it is unreachable?