From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 BF1A246971A for ; Fri, 6 Dec 2019 03:41:49 +0300 (MSK) Date: Fri, 6 Dec 2019 03:41:46 +0300 From: Alexander Turenko Message-ID: <20191206004145.abskgni23bzcujtf@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v1 1/1] lua: allow to push -2^63 in INTEGER field List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org 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? 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. 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. 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 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. > --- > 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.