Re: [Tarantool-patches] [PATCH v1 1/1] lua: allow to push -2^63 in INTEGER field

[Alexander Turenko <alexander.turenko@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.