[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Jul 11 01:49:38 MSK 2019
Thanks for the fixes!
>> --------------------------------------------------------------
>> 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.
> {
> - 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?
> + } else {
> + mem_set_i64(pVal, (int64_t)(-pVal->u.u));
> + }
> }
> sql_value_apply_type(pVal, type);
> }
>
Note for Kirill: this is not a final review. I will get back a bit later to
check other places where Mem.u.u, Mem.u.i, MEM_UInt, MEM_Int are used.
More information about the Tarantool-patches
mailing list