Re: [Tarantool-patches] [PATCH v1 1/1] sql: allow to convert big real values to integer

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for review. My answers and new patch below.
I didn't include diff between patches since patch is quite
short.

On Thu, Nov 21, 2019 at 04:19:55AM +0300, Nikita Pettik wrote:
> On 05 Nov 20:42, imeevma@tarantool.org wrote:
> > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > index 2d37b62..82daac7 100644
> > --- a/src/box/sql/vdbemem.c
> > +++ b/src/box/sql/vdbemem.c
> > @@ -564,10 +564,16 @@ sqlVdbeMemIntegerify(Mem * pMem, bool is_forced)
> >  		mem_set_int(pMem, i, is_neg);
> >  		return 0;
> >  	} else if ((pMem->flags & MEM_Real) != 0 && is_forced) {
> > -		if (pMem->u.r >= INT64_MAX || pMem->u.r < INT64_MIN)
> > -			return -1;
> > -		mem_set_int(pMem, pMem->u.r, pMem->u.r <= -1);
> > -		return 0;
> > +		if (pMem->u.r < INT64_MAX && pMem->u.r >= INT64_MIN) {
> > +			mem_set_int(pMem, pMem->u.r, pMem->u.r <= -1);
> > +			return 0;
> > +		}
> > +		if (pMem->u.r >= INT64_MAX && pMem->u.r < UINT64_MAX) {
> > +			uint64_t val = pMem->u.r;
> > +			mem_set_int(pMem, (int64_t)val, false);
> 
> Use mem_set_u64() instead.
>
Thank, fixed.

> BTW below I see path which is executed when
> is_forced == false. And it is also about same wrong conversion. Please
> check it as well and add test case.
> 
I found that is_forced variable did nothing it this code.
Removed it from code along with "if" checked it.

> > +			return 0;
> > +		}
> > +		return -1;
> >  	}
> >  
> >  	double d;
> > diff --git a/test/sql/integer-overflow.test.lua b/test/sql/integer-overflow.test.lua
> > index 1b3e8ce..5260639 100644
> > --- a/test/sql/integer-overflow.test.lua
> > +++ b/test/sql/integer-overflow.test.lua
> > @@ -32,7 +32,7 @@ box.execute('SELECT CAST(\'18446744073709551616\' AS INTEGER);')
> >  -- float 9223372036854775800 -> int (9223372036854775808),
> >  -- with error due to conversion = 8.
> >  --
> > -box.execute('SELECT CAST(9223372036854775807.0 AS INTEGER);')
> > +box.execute('SELECT CAST(18446744073709551616. AS INTEGER);')
> 
> Why did you changed this test?
> 
Fixed comment before the test. It says, that we should
receive en error.

> >  -- gh-3810: make sure that if space contains integers in range
> >  -- [INT64_MAX, UINT64_MAX], they are handled inside SQL in a
> >  -- proper way, which now means that an error is raised.
> > -- 
> > 2.7.4
> > 

New patch:


From 529a444bba62ccdf9b20896f6c04b2ce7ad2ba82 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Tue, 5 Nov 2019 20:17:52 +0300
Subject: [PATCH] sql: allow to convert big real values to integer

This patch fixes a bug that prevented the conversion of real
values that are greater than INT64_MAX and less than UINT64_MAX to
INTEGER and UNSIGNED.

Closes #4526

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ab86be9..450e21d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -322,13 +322,16 @@ mem_apply_type(struct Mem *record, enum field_type type)
 		if ((record->flags & MEM_UInt) == MEM_UInt)
 			return 0;
 		if ((record->flags & MEM_Real) == MEM_Real) {
-			int64_t i = (int64_t) record->u.r;
-			if (i == record->u.r)
-				mem_set_int(record, record->u.r,
-					    record->u.r <= -1);
+			double d = record->u.r;
+			int64_t i = (int64_t) d;
+			uint64_t u = (uint64_t) d;
+			if (i == d)
+				mem_set_int(record, d, d <= -1);
+			else if (u == d)
+				mem_set_u64(record, d);
 			return 0;
 		}
-		if (sqlVdbeMemIntegerify(record, false) != 0)
+		if (sqlVdbeMemIntegerify(record) != 0)
 			return -1;
 		if ((record->flags & MEM_Int) == MEM_Int) {
 			if (type == FIELD_TYPE_UNSIGNED)
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 0f32b4c..cc973dd 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -506,7 +506,7 @@ int sqlVdbeMemMakeWriteable(Mem *);
 int sqlVdbeMemStringify(Mem *);
 int sqlVdbeIntValue(Mem *, int64_t *, bool *is_neg);
 
-int sqlVdbeMemIntegerify(Mem *, bool is_forced);
+int sqlVdbeMemIntegerify(Mem *);
 int sqlVdbeRealValue(Mem *, double *);
 
 int
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2d37b62..c584ec2 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -554,7 +554,7 @@ mem_apply_integer_type(Mem *pMem)
  * Convert pMem to type integer.  Invalidate any prior representations.
  */
 int
-sqlVdbeMemIntegerify(Mem * pMem, bool is_forced)
+sqlVdbeMemIntegerify(Mem * pMem)
 {
 	assert(EIGHT_BYTE_ALIGNMENT(pMem));
 
@@ -563,20 +563,20 @@ sqlVdbeMemIntegerify(Mem * pMem, bool is_forced)
 	if (sqlVdbeIntValue(pMem, &i, &is_neg) == 0) {
 		mem_set_int(pMem, i, is_neg);
 		return 0;
-	} else if ((pMem->flags & MEM_Real) != 0 && is_forced) {
-		if (pMem->u.r >= INT64_MAX || pMem->u.r < INT64_MIN)
-			return -1;
-		mem_set_int(pMem, pMem->u.r, pMem->u.r <= -1);
-		return 0;
 	}
 
 	double d;
 	if (sqlVdbeRealValue(pMem, &d) != 0)
 		return -1;
-	if (d >= INT64_MAX || d < INT64_MIN)
-		return -1;
-	mem_set_int(pMem, d, d <= -1);
-	return 0;
+	if (d < INT64_MAX && d >= INT64_MIN) {
+		mem_set_int(pMem, d, d <= -1);
+		return 0;
+	}
+	if (d >= INT64_MAX && d < UINT64_MAX) {
+		mem_set_u64(pMem, d);
+		return 0;
+	}
+	return -1;
 }
 
 /*
@@ -735,7 +735,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 			MemSetTypeFlag(pMem, MEM_UInt);
 			return 0;
 		}
-		if (sqlVdbeMemIntegerify(pMem, true) != 0)
+		if (sqlVdbeMemIntegerify(pMem) != 0)
 			return -1;
 		if (type == FIELD_TYPE_UNSIGNED &&
 		    (pMem->flags & MEM_UInt) == 0)
diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
index 9f72485..07117d0 100755
--- a/test/sql-tap/numcast.test.lua
+++ b/test/sql-tap/numcast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(11)
+test:plan(20)
 
 --!./tcltestrunner.lua
 -- 2013 March 20
@@ -65,5 +65,86 @@ for _, enc in ipairs({"utf8"}) do
     end
 end
 
+--
+-- gh-4526: make sure that DOUBLE values that more than
+-- 9223372036854775296 and less than 18446744073709551616 can be
+-- converted to INTEGER or UNSIGNED.
+--
+test:do_execsql_test(
+    "cast-2.1",
+    [[
+        SELECT CAST((9223372036854775297.01) AS INTEGER);
+    ]], {
+        9223372036854775808ULL
+    })
+
+test:do_execsql_test(
+    "cast-2.2",
+    [[
+        SELECT CAST((18000000000000000000.) AS INTEGER);
+    ]], {
+        18000000000000000000ULL
+    })
+
+test:do_execsql_test(
+    "cast-2.3",
+    [[
+        SELECT CAST((9223372036854775297.01) AS UNSIGNED);
+    ]], {
+        9223372036854775808ULL
+    })
+
+test:do_execsql_test(
+    "cast-2.4",
+    [[
+        SELECT CAST((18000000000000000000.) AS UNSIGNED);
+    ]], {
+        18000000000000000000ULL
+    })
+
+test:do_catchsql_test(
+    "cast-2.5",
+    [[
+        SELECT CAST((20000000000000000000.) AS UNSIGNED);
+    ]], {
+        1,"Type mismatch: can not convert 2.0e+19 to unsigned"
+    })
+
+test:do_execsql_test(
+    "cast-2.6",
+    [[
+        CREATE TABLE t (i INTEGER PRIMARY KEY);
+        INSERT INTO t VALUES(9223372036854775297.01);
+        SELECT * FROM t;
+    ]], {
+        9223372036854775808ULL
+    })
+
+test:do_execsql_test(
+    "cast-2.7",
+    [[
+        INSERT INTO t VALUES(18000000000000000000.01);
+        SELECT * FROM t;
+    ]], {
+        9223372036854775808ULL,18000000000000000000ULL
+    })
+
+test:do_catchsql_test(
+    "cast-2.8",
+    [[
+        INSERT INTO t VALUES(20000000000000000000.01);
+        SELECT * FROM t;
+    ]], {
+        1,"Tuple field 1 type does not match one required by operation: expected integer"
+    })
+
+test:do_catchsql_test(
+    "cast-2.9",
+    [[
+        INSERT INTO t VALUES(2.1);
+        SELECT * FROM t;
+    ]], {
+        1,"Tuple field 1 type does not match one required by operation: expected integer"
+    })
 
 test:finish_test()
diff --git a/test/sql/integer-overflow.result b/test/sql/integer-overflow.result
index 223ba02..d6223ff 100644
--- a/test/sql/integer-overflow.result
+++ b/test/sql/integer-overflow.result
@@ -110,15 +110,15 @@ box.execute('SELECT CAST(\'18446744073709551616\' AS INTEGER);')
 - 'Type mismatch: can not convert 18446744073709551616 to integer'
 ...
 -- Due to inexact represantation of large integers in terms of
--- floating point numbers, numerics with value < INT64_MAX
--- have INT64_MAX + 1 value in integer representation:
--- float 9223372036854775800 -> int (9223372036854775808),
--- with error due to conversion = 8.
+-- floating point numbers, numerics with value < UINT64_MAX
+-- have UINT64_MAX + 1 value in integer representation:
+-- float 18446744073709551600 -> int (18446744073709551616),
+-- with error due to conversion = 16.
 --
-box.execute('SELECT CAST(9223372036854775807.0 AS INTEGER);')
+box.execute('SELECT CAST(18446744073709551600. AS INTEGER);')
 ---
 - null
-- 'Type mismatch: can not convert 9.22337203685478e+18 to integer'
+- 'Type mismatch: can not convert 1.84467440737096e+19 to integer'
 ...
 -- gh-3810: make sure that if space contains integers in range
 -- [INT64_MAX, UINT64_MAX], they are handled inside SQL in a
diff --git a/test/sql/integer-overflow.test.lua b/test/sql/integer-overflow.test.lua
index 1b3e8ce..17051f9 100644
--- a/test/sql/integer-overflow.test.lua
+++ b/test/sql/integer-overflow.test.lua
@@ -27,12 +27,12 @@ box.execute('SELECT 18446744073709551616;')
 box.execute('SELECT CAST(\'9223372036854775808\' AS INTEGER);')
 box.execute('SELECT CAST(\'18446744073709551616\' AS INTEGER);')
 -- Due to inexact represantation of large integers in terms of
--- floating point numbers, numerics with value < INT64_MAX
--- have INT64_MAX + 1 value in integer representation:
--- float 9223372036854775800 -> int (9223372036854775808),
--- with error due to conversion = 8.
+-- floating point numbers, numerics with value < UINT64_MAX
+-- have UINT64_MAX + 1 value in integer representation:
+-- float 18446744073709551600 -> int (18446744073709551616),
+-- with error due to conversion = 16.
 --
-box.execute('SELECT CAST(9223372036854775807.0 AS INTEGER);')
+box.execute('SELECT CAST(18446744073709551600. AS INTEGER);')
 -- gh-3810: make sure that if space contains integers in range
 -- [INT64_MAX, UINT64_MAX], they are handled inside SQL in a
 -- proper way, which now means that an error is raised.