[Tarantool-patches] [PATCH v3 0/2] Allow to convert big real values to integer

[Tarantool-patches] [PATCH v3 0/2] Allow to convert big real values to integer

[imeevma]

This patch-set 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.

https://github.com/tarantool/tarantool/issues/4526
https://github.com/tarantool/tarantool/tree/imeevma/gh-4526-big-float-to-int-conversation

Mergen Imeev (2):
  sql: refactor sqlVdbeMemIntegerify() function
  sql: allow to convert big real values to integer

 src/box/sql/vdbe.c                 | 13 +++---
 src/box/sql/vdbeInt.h              |  2 +-
 src/box/sql/vdbemem.c              | 22 +++++-----
 test/sql-tap/numcast.test.lua      | 83 +++++++++++++++++++++++++++++++++++++-
 test/sql/integer-overflow.result   | 12 +++---
 test/sql/integer-overflow.test.lua | 10 ++---
 6 files changed, 113 insertions(+), 29 deletions(-)

-- 
2.7.4

[Tarantool-patches] [PATCH v3 1/2] sql: refactor sqlVdbeMemIntegerify() function

[imeevma]

The sqlVdbeMemIntegerify() function accepts the is_forced flag as
one of the arguments. But this flag is actually useless, because
regardless of whether it is TRUE or FALSE, the function will do
the same. This patch removes the is_forced argument from the
function. There will be no test, since the result of the function
has not changed.

Part of #4526
---
 src/box/sql/vdbe.c    | 2 +-
 src/box/sql/vdbeInt.h | 2 +-
 src/box/sql/vdbemem.c | 9 ++-------
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index ab86be9..8ea1df8 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -328,7 +328,7 @@ mem_apply_type(struct Mem *record, enum field_type type)
 					    record->u.r <= -1);
 			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..d70f683 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(struct Mem *pMem);
 int sqlVdbeRealValue(Mem *, double *);
 
 int
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 2d37b62..e2e2b7a 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(struct Mem *pMem)
 {
 	assert(EIGHT_BYTE_ALIGNMENT(pMem));
 
@@ -563,11 +563,6 @@ 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;
@@ -735,7 +730,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)
-- 
2.7.4

[Tarantool-patches] [PATCH v3 2/2] sql: allow to convert big real values to integer

[imeevma]

Hi! Thank you for review! My answers and new patch below.

On 12/5/19 3:36 PM, Nikita Pettik wrote:
> On 23 Nov 14:48, Mergen Imeev wrote:
>>> 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.
>
> If it is unused, please move this refactoring to a separate patch.
>
Done.

>> 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);
>
> Nit: mem_set_int(record, i, i <= -1;);
>
Fixed.

>> +			else if (u == d)
>> +				mem_set_u64(record, d);
>
> Nit: mem_set_u64(record, u);
>
Fixed.

New patch:

From b4e446e1e7a0309e2e20ebdef84feff73805bb59 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 8ea1df8..6ebc5f0 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -322,10 +322,13 @@ 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, i, i <= -1);
+			else if (u == d)
+				mem_set_u64(record, u);
 			return 0;
 		}
 		if (sqlVdbeMemIntegerify(record) != 0)
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index e2e2b7a..407b42e 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -568,10 +568,15 @@ sqlVdbeMemIntegerify(struct Mem *pMem)
 	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;
 }
 
 /*
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.

Re: [Tarantool-patches] [PATCH v3 0/2] Allow to convert big real values to integer

[Nikita Pettik]

On 09 Dec 16:34, imeevma@tarantool.org wrote:
> This patch-set 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.

LGTM. Pushed to master.
 
> https://github.com/tarantool/tarantool/issues/4526
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4526-big-float-to-int-conversation
> 
> Mergen Imeev (2):
>   sql: refactor sqlVdbeMemIntegerify() function
>   sql: allow to convert big real values to integer
> 
>  src/box/sql/vdbe.c                 | 13 +++---
>  src/box/sql/vdbeInt.h              |  2 +-
>  src/box/sql/vdbemem.c              | 22 +++++-----
>  test/sql-tap/numcast.test.lua      | 83 +++++++++++++++++++++++++++++++++++++-
>  test/sql/integer-overflow.result   | 12 +++---
>  test/sql/integer-overflow.test.lua | 10 ++---
>  6 files changed, 113 insertions(+), 29 deletions(-)
> 
> -- 
> 2.7.4
>