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

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

>>> --------------------------------------------------------------
>>> 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?

>> {
>> -       if (sizeof(LONGDOUBLE_TYPE) > 8) {
>> -               LONGDOUBLE_TYPE x = (LONGDOUBLE_TYPE) i;
>> -               if (x < r)
>> -                       return -1;
>> -               if (x > r)
>> -                       return +1;
>> -               return 0;
>> -       } else {
>> -               i64 y;
>> -               double s;
>> -               if (r < -9223372036854775808.0)
>> -                       return +1;
>> -               if (r > 9223372036854775807.0)
>> -                       return -1;
>> -               y = (i64) r;
>> -               if (i < y)
>> -                       return -1;
>> -               if (i > y) {
>> -                       if (y == SMALLEST_INT64 && r > 0.0)
>> -                               return -1;
>> -                       return +1;
>> -               }
>> -               s = (double)i;
>> -               if (s < r)
>> -                       return -1;
>> -               if (s > r)
>> -                       return +1;
>> -               return 0;
>> -       }
>> +       if (r > (double) UINT64_MAX)
>> +               return -1;
>> +       if (r < 0.0)
>> +               return +1;
>> +       uint64_t y = (uint64_t) r;
>> +       if (u < y)
>> +               return -1;
>> +       if (u > y)
>> +               return +1;
>> +       double s = (double) u;
>> +       if (s < r)
>> +               return -1;
>> +       if (s > r)
>> +               return +1;
>> +       return 0;
>> +}
>> +
>> +static int
>> +compare_int_float(int64_t i, double r)
> 
> It has the same problems as the previous function,
> but can be fixed by calling compare_uint_float()
> with a modulo of 'int64_t i' and reversed result,
> if the value was negative. This is what mp_compare_double_any_int
> does.
> 
>> +{
>> +       if (r < (double) INT64_MIN)
>> +               return +1;
>> +       if (r >= 0.0)
>> +               return -1;
>> +       int64_t y = (int64_t) r;
>> +       if (i < y)
>> +               return -1;
>> +       if (i > y)
>> +               return +1;
>> +       double s = (double) i;
>> +       if (s < r)
>> +               return -1;
>> +       if (s > r)
>> +               return +1;
>> +       return 0;
>> }
>>> --------------------------------------------------------------
>>> vdbemem.c:1431:
>>> 			} else if (pVal->u.i == SMALLEST_INT64) {
>>> 				pVal->u.r = -(double)SMALLEST_INT64;
>>> 				MemSetTypeFlag(pVal, MEM_Real);
>>> 			} else {
>>> 				pVal->u.i = -pVal->u.i;
>>> 			}
>>> 
>>> You compare u.i and SMALLEST_INT64, but you can't
>>> be sure, that u.i is not a big unsigned, can you?
>> 
>> Fixed:
>> 
>> 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()..