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 52BF822D51 for ; Thu, 18 Jul 2019 16:16:38 -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 Jw2bqKbigtav for ; Thu, 18 Jul 2019 16:16:38 -0400 (EDT) Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [217.69.128.43]) (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 96DB522D2D for ; Thu, 18 Jul 2019 16:16:37 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type References: <734EC309-6DCF-42C2-8041-135A8B68E935@tarantool.org> <9a397d31-1cae-0dd0-cdd6-733388cb01af@tarantool.org> <34798AF0-1069-461A-A8EA-47385B4B682D@tarantool.org> From: Vladislav Shpilevoy Message-ID: <15ef6d04-f8b5-b9b4-a160-7c6f294d2a00@tarantool.org> Date: Thu, 18 Jul 2019 22:18:16 +0200 MIME-Version: 1.0 In-Reply-To: <34798AF0-1069-461A-A8EA-47385B4B682D@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! 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?