Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: v.shpilevoy@tarantool.org, Nikita Pettik <korablev@tarantool.org>
Subject: [tarantool-patches] [PATCH 2/2] sql: use double_compare_uint64() for int<->float cmp
Date: Wed,  7 Aug 2019 01:02:15 +0300	[thread overview]
Message-ID: <c89df31e03ba5e047f61864ebd65c9ee43b32592.1565120194.git.korablev@tarantool.org> (raw)
In-Reply-To: <cover.1565120194.git.korablev@tarantool.org>
In-Reply-To: <cover.1565120194.git.korablev@tarantool.org>

To compare floating point values and integers in SQL functions
compare_uint_float() and compare_int_float() are used. Unfortunately,
they contain bug connected with checking border case: that's not correct
to cast UINT64_MAX (2^64 - 1) to double. Proper way is to use exp2(2^64)
or predefined floating point constant. To not bother fixing function
which in turn may contain other tricky places, let's use instead already
verified double_compare_uint64(). So that we have unified way of
integer<->float comparison.
---
 src/box/sql/vdbeaux.c | 74 +++++++++++----------------------------------------
 test/sql/types.result |  2 +-
 2 files changed, 17 insertions(+), 59 deletions(-)

diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e7accc745..2654f5c7a 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2886,52 +2886,6 @@ sqlBlobCompare(const Mem * pB1, const Mem * pB2)
 	return n1 - n2;
 }
 
-/**
- * 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
-compare_uint_float(uint64_t u, double r)
-{
-	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)
-{
-	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;
-}
-
 /*
  * Compare the values contained by the two memory cells, returning
  * negative, zero or positive if pMem1 is less than, equal to, or greater
@@ -2997,16 +2951,16 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
 		}
 		if ((f1 & MEM_Int) != 0) {
 			if ((f2 & MEM_Real) != 0) {
-				return compare_int_float(pMem1->u.i,
-							 pMem2->u.r);
+				return double_compare_uint64(-pMem2->u.r,
+							     -pMem1->u.i, 1);
 			} else {
 				return -1;
 			}
 		}
 		if ((f1 & MEM_UInt) != 0) {
 			if ((f2 & MEM_Real) != 0) {
-				return compare_uint_float(pMem1->u.u,
-							  pMem2->u.r);
+				return double_compare_uint64(pMem2->u.r,
+							     pMem1->u.u, -1);
 			} else if ((f2 & MEM_Int) != 0) {
 				return +1;
 			} else {
@@ -3015,11 +2969,11 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
 		}
 		if ((f1 & MEM_Real) != 0) {
 			if ((f2 & MEM_Int) != 0) {
-				return -compare_int_float(pMem2->u.i,
-							  pMem1->u.r);
+				return double_compare_uint64(-pMem1->u.r,
+							     -pMem2->u.i, -1);
 			} else if ((f2 & MEM_UInt) != 0) {
-				return -compare_uint_float(pMem2->u.u,
-							   pMem1->u.r);
+				return double_compare_uint64(pMem1->u.r,
+							     pMem2->u.u, 1);
 			} else {
 				return -1;
 			}
@@ -3188,7 +3142,8 @@ sqlVdbeCompareMsgpack(const char **key1,
 				else if (mem1.u.u > pKey2->u.u)
 					rc = +1;
 			} else if ((pKey2->flags & MEM_Real) != 0) {
-				rc = compare_uint_float(mem1.u.u, pKey2->u.r);
+				rc = double_compare_uint64(pKey2->u.r,
+							   mem1.u.u, -1);
 			} else {
 				rc = (pKey2->flags & MEM_Null) ? +1 : -1;
 			}
@@ -3205,7 +3160,8 @@ sqlVdbeCompareMsgpack(const char **key1,
 					rc = +1;
 				}
 			} else if (pKey2->flags & MEM_Real) {
-				rc = compare_int_float(mem1.u.i, pKey2->u.r);
+				rc = double_compare_uint64(-pKey2->u.r,
+							   -mem1.u.i, 1);
 			} else {
 				rc = (pKey2->flags & MEM_Null) ? +1 : -1;
 			}
@@ -3219,9 +3175,11 @@ sqlVdbeCompareMsgpack(const char **key1,
 			mem1.u.r = mp_decode_double(&aKey1);
  do_float:
 			if ((pKey2->flags & MEM_Int) != 0) {
-				rc = -compare_int_float(pKey2->u.i, mem1.u.r);
+				rc = double_compare_uint64(-mem1.u.r,
+							   -pKey2->u.i, -1);
 			} else if (pKey2->flags & MEM_UInt) {
-				rc = -compare_uint_float(pKey2->u.u, mem1.u.r);
+				rc = double_compare_uint64(mem1.u.r,
+							   pKey2->u.u, 1);
 			} else if (pKey2->flags & MEM_Real) {
 				if (mem1.u.r < pKey2->u.r) {
 					rc = -1;
diff --git a/test/sql/types.result b/test/sql/types.result
index 83820af53..c95d21e0f 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1286,7 +1286,7 @@ box.execute("SELECT 18446744073709551615.0 > 18446744073709551615")
   - name: 18446744073709551615.0 > 18446744073709551615
     type: boolean
   rows:
-  - [false]
+  - [true]
 ...
 box.execute("SELECT 18446744073709551615 IN ('18446744073709551615', 18446744073709551615.0)")
 ---
-- 
2.15.1

  parent reply	other threads:[~2019-08-06 22:02 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-06 22:02 [tarantool-patches] [PATCH 0/2] Fix border case in float-int comparison in SQL Nikita Pettik
2019-08-06 22:02 ` [tarantool-patches] [PATCH 1/2] Move mp_compare_double_uint64() to trivia.h Nikita Pettik
2019-08-08 21:37   ` [tarantool-patches] ***UNCHECKED*** " Vladislav Shpilevoy
2019-08-14 10:36     ` [tarantool-patches] " n.pettik
2019-08-14 20:43       ` Vladislav Shpilevoy
2019-08-08 21:37   ` [tarantool-patches] ***UNCHECKED*** " Vladislav Shpilevoy
2019-08-06 22:02 ` Nikita Pettik [this message]
2019-08-22 12:31 ` [tarantool-patches] Re: [PATCH 0/2] Fix border case in float-int comparison in SQL Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c89df31e03ba5e047f61864ebd65c9ee43b32592.1565120194.git.korablev@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [tarantool-patches] [PATCH 2/2] sql: use double_compare_uint64() for int<->float cmp' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox