[Tarantool-patches] [PATCH v2 1/1] lua/serializer: encode Lua number -2^63 as integer
imeevma at tarantool.org
imeevma at tarantool.org
Wed Dec 11 13:49:02 MSK 2019
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 at 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 at 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()
More information about the Tarantool-patches
mailing list