From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 7CFAB46970F for ; Sat, 23 Nov 2019 14:48:08 +0300 (MSK) Date: Sat, 23 Nov 2019 14:48:06 +0300 From: Mergen Imeev Message-ID: <20191123114805.GA18119@tarantool.org> References: <607d553a38e634e3e1dd9d8fe2bb7be03effc8d3.1572975628.git.imeevma@gmail.com> <20191121011955.GA4142@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20191121011955.GA4142@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: allow to convert big real values to integer List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.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 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.