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 264292531E for ; Wed, 10 Jul 2019 18:48:18 -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 vNYmryDFH4fT for ; Wed, 10 Jul 2019 18:48:18 -0400 (EDT) Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 D8A552531C for ; Wed, 10 Jul 2019 18:48:17 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type References: <734EC309-6DCF-42C2-8041-135A8B68E935@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9a397d31-1cae-0dd0-cdd6-733388cb01af@tarantool.org> Date: Thu, 11 Jul 2019 00:49:38 +0200 MIME-Version: 1.0 In-Reply-To: <734EC309-6DCF-42C2-8041-135A8B68E935@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: "n.pettik" , tarantool-patches@freelists.org 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.