From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E5BA023478 for ; Tue, 16 Jul 2019 20:54:44 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id ZWzY6KwFRqDH for ; Tue, 16 Jul 2019 20:54:44 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id A47B223455 for ; Tue, 16 Jul 2019 20:54:44 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type From: "n.pettik" In-Reply-To: <9a397d31-1cae-0dd0-cdd6-733388cb01af@tarantool.org> Date: Wed, 17 Jul 2019 03:54:41 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <34798AF0-1069-461A-A8EA-47385B4B682D@tarantool.org> References: <734EC309-6DCF-42C2-8041-135A8B68E935@tarantool.org> <9a397d31-1cae-0dd0-cdd6-733388cb01af@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >>> -------------------------------------------------------------- >>> vdbeaux.c:2998: >>> if ((f1 & MEM_UInt) !=3D 0) { >>> if ((f2 & MEM_Real) !=3D 0) { >>> return sqlIntFloatCompare(pMem1->u.i, >>>=20 >>> pMem1 is unsigned, according to the first check, >>> but you use u.i. Why? >>=20 >> Thx, I=E2=80=99ve fixed series of similar places and extended = sql/types.test.lua: >>=20 >> 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; >> } >>=20 >> -/* >> - * 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) >=20 > 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=E2=80=99m 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=E2=80=99t you mind if I send it as follow-up after = this patch-set hits the trunk? >> { >> - if (sizeof(LONGDOUBLE_TYPE) > 8) { >> - LONGDOUBLE_TYPE x =3D (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 =3D (i64) r; >> - if (i < y) >> - return -1; >> - if (i > y) { >> - if (y =3D=3D SMALLEST_INT64 && r > 0.0) >> - return -1; >> - return +1; >> - } >> - s =3D (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 =3D (uint64_t) r; >> + if (u < y) >> + return -1; >> + if (u > y) >> + return +1; >> + double s =3D (double) u; >> + if (s < r) >> + return -1; >> + if (s > r) >> + return +1; >> + return 0; >> +} >> + >> +static int >> +compare_int_float(int64_t i, double r) >=20 > 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. >=20 >> +{ >> + if (r < (double) INT64_MIN) >> + return +1; >> + if (r >=3D 0.0) >> + return -1; >> + int64_t y =3D (int64_t) r; >> + if (i < y) >> + return -1; >> + if (i > y) >> + return +1; >> + double s =3D (double) i; >> + if (s < r) >> + return -1; >> + if (s > r) >> + return +1; >> + return 0; >> } >>> -------------------------------------------------------------- >>> vdbemem.c:1431: >>> } else if (pVal->u.i =3D=3D SMALLEST_INT64) { >>> pVal->u.r =3D -(double)SMALLEST_INT64; >>> MemSetTypeFlag(pVal, MEM_Real); >>> } else { >>> pVal->u.i =3D -pVal->u.i; >>> } >>>=20 >>> You compare u.i and SMALLEST_INT64, but you can't >>> be sure, that u.i is not a big unsigned, can you? >>=20 >> Fixed: >>=20 >> 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 =3D -pVal->u.r; >> - } else if (pVal->u.i =3D=3D SMALLEST_INT64) { >> - pVal->u.r =3D = -(double)SMALLEST_INT64; >> - MemSetTypeFlag(pVal, MEM_Real); >> - } else { >> - pVal->u.i =3D -pVal->u.i; >> + } else if ((pVal->flags & MEM_Int) !=3D 0) { >> + mem_set_u64(pVal, = (uint64_t)(-pVal->u.i)); >> + } else if ((pVal->flags & MEM_UInt) !=3D 0) { >> + if (pVal->u.u > (uint64_t) INT64_MAX = + 1) { >> + pVal->u.r =3D -(double) = pVal->u.u; >> + MemSetTypeFlag(pVal, = MEM_Real); >=20 > 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=E2=80=99s 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()..