[Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer

[Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer

[imeevma]

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

https://github.com/tarantool/tarantool/issues/4672
https://github.com/tarantool/tarantool/tree/imeevma/gh-4672-fix-min-integer-in-lua

On 12/6/19 3:41 AM, Alexander Turenko wrote:
> Nice catch!
>
> I think it worth to file an issue. The problem looks small, but it would
> be better to let users aware of it.
>
> Let's describe where the problem can appear: msgpack module (and so box
> functions, binary protocol responses -- everything that encodes lua
> values to msgpack except msgpackffi and so except tuple.update), json
> and yaml modules. Describe buggy and desired behaviour for -2^63:
>
> (I use type names from [msgpack spec][1] rather then our sizeless MP_*.)
>
> * msgpack module (the same for iproto responses I guess): expected
>   int64, got float64.
> * box (insert into an integer column): expected success, got an error.
> * json / yaml modules (and so console output): expected precise value
>   -9223372036854775808, got approximated one in scientific notation
>   -9.2233720368548e+18.
>
> If you know other cases, let's add them too. Maybe something in SQL?
>
Done. SQL do not use this serializer, so it doesn't have this
problem.

> I would add all those cases to corresponding tests: re msgpack, re
> msgpackffi, re json, re yaml, re iproto, re console, re box's insert, re
> tuple.update, (something else?). Yep, don't miss msgpackffi and
> tuple.update -- its behaviour is right, but a case is worthful anyway to
> catch possible future regressions.
>
I added a new test in serializer_test. Since this module is used
in the json, yaml, msgpack and msgpackffi tests, I think we do not
need any other tests for these modules.

> Let's consult with Kirill Yu. whether we should add those cases to
> existing files, add all them into one file or create a file for each
> one. Personally I vote for the first way.
>
Kirill said to create one test for each test-suite. Since we no
longer need tests in app-tap, we only need one test in box-tap.
However, I created a test in box instead of box-tap, as it was a
little easier.

> See other comments below.
>
> [1]: https://github.com/msgpack/msgpack/blob/master/spec.md
>
> WBR, Alexander Turenko.
>
>> lua: allow to push -2^63 in INTEGER field
>
> Nit: This header is a bit misleading: prefix states that it is about
> lua, but 'integer field' is more about box.
>
> I would give a general description in the header, like so:
>
>> lua/serializer: encode Lua number -2^63 as integer
>
Fixed.

> And would give all details about particular cases in the commit message.
>
> On Thu, Nov 28, 2019 at 05:11:49PM +0300, imeevma@tarantool.org wrote:
>> This patch fixes a bug that appeared due to commit 3a13be1d.
>
> Nit: We usually use form 3a13be1de38e79b6b7e0e7a6fa81f4c59c379f4e ('lua:
> fix lua_tofield() for 2**64 value') for a reference to a commit.
>
Done. However, in the log and on github this looks different since
github automatically shortens the commit id.

>> ---
>> https://github.com/tarantool/tarantool/tree/imeevma/small_fixes
>>
>>  src/lua/utils.c             |  2 +-
>>  test/engine/insert.result   | 28 ++++++++++++++++++++++++++++
>>  test/engine/insert.test.lua |  8 ++++++++
>>  3 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/lua/utils.c b/src/lua/utils.c
>> index 7f7bf47..bad5f7b 100644
>> --- a/src/lua/utils.c
>> +++ b/src/lua/utils.c
>> @@ -667,7 +667,7 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
>>  		} else if (num >= 0 && num < exp2(64)) {
>>  			field->type = MP_UINT;
>>  			field->ival = (uint64_t) num;
>> -		} else if (num > -exp2(63) && num < exp2(63)) {
>> +		} else if (num >= -exp2(63) && num < exp2(63)) {
>>  			field->type = MP_INT;
>>  			field->ival = (int64_t) num;
>>  		} else {
>
>> diff --git a/test/engine/insert.test.lua b/test/engine/insert.test.lua
>> index 39b1501..d6df60e 100644
>> --- a/test/engine/insert.test.lua
>> +++ b/test/engine/insert.test.lua
>> @@ -123,3 +123,11 @@ s:select{}
>>  s:drop()
>>  fiber = nil
>>  
>> +format = {{name = 'i', type = 'integer'}}
>> +s = box.schema.space.create('s', {format = format, engine = engine})
>> +_ = s:create_index('ii')
>> +s:insert({-2^63})
>> +s:insert({-2^64})
>> +s:insert({2^63})
>> +s:insert({2^64})
>> +s:drop()
>
> Nit: Let's add 'gh-xxxx: short description' above the test case.
Done.


New patch:

From 0e3de775c20f0e5776301a5b39fa1e22e0e8bdb7 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Fri, 6 Dec 2019 19:22:40 +0300
Subject: [PATCH] lua/serializer: encode Lua number -2^63 as integer

This patch fixes a bug that appeared after commit
3a13be1de38e79b6b7e0e7a6fa81f4c59c379f4e ('lua: fix lua_tofield()
for 2**64 value') .

Due to this bug, -2^63 was serialized as double, although it
should be serialized as integer.

Closes #4672

diff --git a/src/lua/utils.c b/src/lua/utils.c
index d5122ee..208b64e 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -667,7 +667,7 @@ luaL_tofield(struct lua_State *L, struct luaL_serializer *cfg, int index,
 		} else if (num >= 0 && num < exp2(64)) {
 			field->type = MP_UINT;
 			field->ival = (uint64_t) num;
-		} else if (num > -exp2(63) && num < exp2(63)) {
+		} else if (num >= -exp2(63) && num < exp2(63)) {
 			field->type = MP_INT;
 			field->ival = (int64_t) num;
 		} else {
diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua
index 1609f76..0385597 100644
--- a/test/app-tap/lua/serializer_test.lua
+++ b/test/app-tap/lua/serializer_test.lua
@@ -101,7 +101,7 @@ local function test_unsigned(test, s)
 end
 
 local function test_signed(test, s)
-    test:plan(52)
+    test:plan(53)
 
     rt(test, s, -1, 'number')
     rt(test, s, -1LL, 'number')
@@ -143,6 +143,10 @@ local function test_signed(test, s)
 
     rt(test, s, ffi.new('short', -128), 'number')
     rt(test, s, ffi.new('int', -128), 'number')
+
+    -- gh-4672: Make sure that -2^63 encoded as INTEGER.
+    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63),
+            '-2^63 encoded as INTEGER')
 end
 
 local function test_double(test, s)
diff --git a/test/box/gh-4672-min-integer-value-in-serializer.result b/test/box/gh-4672-min-integer-value-in-serializer.result
new file mode 100644
index 0000000..f4d7adb
--- /dev/null
+++ b/test/box/gh-4672-min-integer-value-in-serializer.result
@@ -0,0 +1,97 @@
+-- test-run result file version 2
+remote = require('net.box')
+ | ---
+ | ...
+env = require('test_run')
+ | ---
+ | ...
+test_run = env.new()
+ | ---
+ | ...
+
+--
+-- gh-4672: Due to an error in the serializer, -2^63 was
+-- serialized as double, although in accordance with the rules of
+-- serialization it should be serialized as an integer.
+--
+format={{name='u', type='unsigned'}, {name='i', type='integer'}}
+ | ---
+ | ...
+s = box.schema.space.create('serializer_test_space', {format=format})
+ | ---
+ | ...
+_ = s:create_index('ii')
+ | ---
+ | ...
+
+s:insert({1, -2^63})
+ | ---
+ | - [1, -9223372036854775808]
+ | ...
+s:insert({2, -9223372036854775808LL})
+ | ---
+ | - [2, -9223372036854775808]
+ | ...
+s:insert({3, 0})
+ | ---
+ | - [3, 0]
+ | ...
+s:update(3, {{'=', 'i', -2^63}})
+ | ---
+ | - [3, -9223372036854775808]
+ | ...
+s:insert({4, 0})
+ | ---
+ | - [4, 0]
+ | ...
+s:update(4, {{'=', 'i', -9223372036854775808LL}})
+ | ---
+ | - [4, -9223372036854775808]
+ | ...
+
+box.schema.user.grant('guest', 'read, write', 'space', 'serializer_test_space')
+ | ---
+ | ...
+
+cn = remote.connect(box.cfg.listen)
+ | ---
+ | ...
+s = cn.space.serializer_test_space
+ | ---
+ | ...
+
+s:insert({11, -2^63})
+ | ---
+ | - [11, -9223372036854775808]
+ | ...
+s:insert({12, -9223372036854775808LL})
+ | ---
+ | - [12, -9223372036854775808]
+ | ...
+s:insert({13, 0})
+ | ---
+ | - [13, 0]
+ | ...
+s:update(13, {{'=', 'i', -2^63}})
+ | ---
+ | - [13, -9223372036854775808]
+ | ...
+s:insert({14, 0})
+ | ---
+ | - [14, 0]
+ | ...
+s:update(14, {{'=', 'i', -9223372036854775808LL}})
+ | ---
+ | - [14, -9223372036854775808]
+ | ...
+
+cn:close()
+ | ---
+ | ...
+box.schema.user.revoke('guest', 'read, write', 'space', 'serializer_test_space')
+ | ---
+ | ...
+
+box.space.serializer_test_space:drop()
+ | ---
+ | ...
diff --git a/test/box/gh-4672-min-integer-value-in-serializer.test.lua b/test/box/gh-4672-min-integer-value-in-serializer.test.lua
new file mode 100644
index 0000000..f718a56
--- /dev/null
+++ b/test/box/gh-4672-min-integer-value-in-serializer.test.lua
@@ -0,0 +1,36 @@
+remote = require('net.box')
+env = require('test_run')
+test_run = env.new()
+
+--
+-- gh-4672: Due to an error in the serializer, -2^63 was
+-- serialized as double, although in accordance with the rules of
+-- serialization it should be serialized as an integer.
+--
+format={{name='u', type='unsigned'}, {name='i', type='integer'}}
+s = box.schema.space.create('serializer_test_space', {format=format})
+_ = s:create_index('ii')
+
+s:insert({1, -2^63})
+s:insert({2, -9223372036854775808LL})
+s:insert({3, 0})
+s:update(3, {{'=', 'i', -2^63}})
+s:insert({4, 0})
+s:update(4, {{'=', 'i', -9223372036854775808LL}})
+
+box.schema.user.grant('guest', 'read, write', 'space', 'serializer_test_space')
+
+cn = remote.connect(box.cfg.listen)
+s = cn.space.serializer_test_space
+
+s:insert({11, -2^63})
+s:insert({12, -9223372036854775808LL})
+s:insert({13, 0})
+s:update(13, {{'=', 'i', -2^63}})
+s:insert({14, 0})
+s:update(14, {{'=', 'i', -9223372036854775808LL}})
+
+cn:close()
+box.schema.user.revoke('guest', 'read, write', 'space', 'serializer_test_space')
+
+box.space.serializer_test_space:drop()

Re: [Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer

[Alexander Turenko]

Thanks!

LGTM.

Igor, can you do second review?

WBR, Alexander Turenko.

On Wed, Dec 11, 2019 at 01:49:02PM +0300, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers and new patch below.
> 
> https://github.com/tarantool/tarantool/issues/4672
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4672-fix-min-integer-in-lua

Re: [Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer

[Igor Munkin]

Mergen,

Thanks, the changes are totally LGTM. However, please consider the side
note I left below (I expect no changes in patch regarding it).

Cced Kirill to proceed with with the patch.

On 11.12.19, imeevma@tarantool.org wrote:

<snipped>

> 
> New patch:
> 
> From 0e3de775c20f0e5776301a5b39fa1e22e0e8bdb7 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Fri, 6 Dec 2019 19:22:40 +0300
> Subject: [PATCH] lua/serializer: encode Lua number -2^63 as integer
> 
> This patch fixes a bug that appeared after commit
> 3a13be1de38e79b6b7e0e7a6fa81f4c59c379f4e ('lua: fix lua_tofield()
> for 2**64 value') .
> 
> Due to this bug, -2^63 was serialized as double, although it
> should be serialized as integer.
> 
> Closes #4672
> 

<snipped>

> +
> +    -- gh-4672: Make sure that -2^63 encoded as INTEGER.
> +    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63),
> +            '-2^63 encoded as INTEGER')

Side note: After applying the following diff on your patch, tests are
still fine.
| diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua
| index 038559756..7d3f62109 100644
| --- a/test/app-tap/lua/serializer_test.lua
| +++ b/test/app-tap/lua/serializer_test.lua
| @@ -145,7 +145,7 @@ local function test_signed(test, s)
|      rt(test, s, ffi.new('int', -128), 'number')
|
|      -- gh-4672: Make sure that -2^63 encoded as INTEGER.
| -    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63),
| +    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63 - 1),
|              '-2^63 encoded as INTEGER')
|  end
Yep, it's double precision issue, and we can do nothing here, but I
guess it ought to be well-documented, taking into account the patch
you've made.

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer

[Mergen Imeev]

Hi! Thank you for review. I added a comment about the
issue here:
https://github.com/tarantool/doc/issues/928#issuecomment-567449272

On Thu, Dec 19, 2019 at 12:42:10AM +0300, Igor Munkin wrote:
> Mergen,
> 
> Thanks, the changes are totally LGTM. However, please consider the side
> note I left below (I expect no changes in patch regarding it).
> 
> Cced Kirill to proceed with with the patch.
> 
> On 11.12.19, imeevma@tarantool.org wrote:
> 
> <snipped>
> 
> > 
> > New patch:
> > 
> > From 0e3de775c20f0e5776301a5b39fa1e22e0e8bdb7 Mon Sep 17 00:00:00 2001
> > From: Mergen Imeev <imeevma@gmail.com>
> > Date: Fri, 6 Dec 2019 19:22:40 +0300
> > Subject: [PATCH] lua/serializer: encode Lua number -2^63 as integer
> > 
> > This patch fixes a bug that appeared after commit
> > 3a13be1de38e79b6b7e0e7a6fa81f4c59c379f4e ('lua: fix lua_tofield()
> > for 2**64 value') .
> > 
> > Due to this bug, -2^63 was serialized as double, although it
> > should be serialized as integer.
> > 
> > Closes #4672
> > 
> 
> <snipped>
> 
> > +
> > +    -- gh-4672: Make sure that -2^63 encoded as INTEGER.
> > +    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63),
> > +            '-2^63 encoded as INTEGER')
> 
> Side note: After applying the following diff on your patch, tests are
> still fine.
> | diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua
> | index 038559756..7d3f62109 100644
> | --- a/test/app-tap/lua/serializer_test.lua
> | +++ b/test/app-tap/lua/serializer_test.lua
> | @@ -145,7 +145,7 @@ local function test_signed(test, s)
> |      rt(test, s, ffi.new('int', -128), 'number')
> |
> |      -- gh-4672: Make sure that -2^63 encoded as INTEGER.
> | -    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63),
> | +    test:ok(s.encode(-9223372036854775808LL) == s.encode(-2^63 - 1),
> |              '-2^63 encoded as INTEGER')
> |  end
> Yep, it's double precision issue, and we can do nothing here, but I
> guess it ought to be well-documented, taking into account the patch
> you've made.
> 
> -- 
> Best regards,
> IM

Re: [Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer

[Kirill Yukhin]

Hello,

On 11 дек 13:49, imeevma@tarantool.org wrote:
> Hi! Thank you for review! My answers and new patch below.
> 
> https://github.com/tarantool/tarantool/issues/4672
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4672-fix-min-integer-in-lua

I've checked your patch into 1.10, 2.2 and master.

--
Regards, Kirill Yukhin