Re: [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option

["v.shpilevoy@tarantool.org" <v.shpilevoy@tarantool.org>]

LGTM. Vova, please, take a look.

> 28 марта 2018 г., в 11:05, Kirill Shcherbatov <kshcherbatov@tarantool.org> написал(а):
> 
> -       bool names_only = 0;
> +       bool names_only = false;
> 
> On 28.03.2018 11:01, Kirill Shcherbatov wrote:
>> Accounted your comments; A new test with empty tuple. Rebased on 1.9
>> On 27.03.2018 18:34, v.shpilevoy@tarantool.org wrote:
>>> See below 5 comments.
>>> 
>>>> 27 марта 2018 г., в 18:00, Kirill Shcherbatov <kshcherbatov@tarantool.org> написал(а):
>>>> 
>>>> Fixes #3280
>>>> ---
>>>> src/box/lua/tuple.c        | 24 +++++++++++++++++++--
>>>> test/engine/tuple.result   | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> test/engine/tuple.test.lua | 16 ++++++++++++++
>>>> 3 files changed, 91 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
>>>> index d5ed7c4..b52072c 100644
>>>> --- a/src/box/lua/tuple.c
>>>> +++ b/src/box/lua/tuple.c
>>>> @@ -280,8 +280,21 @@ luamp_encode_extension_box(struct lua_State *L, int idx,
>>>> static int
>>>> lbox_tuple_to_map(struct lua_State *L)
>>>> {
>>>> -    if (lua_gettop(L) < 1)
>>>> -        luaL_error(L, "Usage: tuple:tomap()");
>>>> +    int argc = lua_gettop(L);
>>>> +    if (argc < 1 || argc > 2)
>>>> +        goto error;
>>>> +    int names_only = 0;
>>> 
>>> 1. As I noted in the previous reviews, use 'bool' for flags. Not integer.
>> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
>> index b52072c..00be718 100644
>> --- a/src/box/lua/tuple.c
>> +++ b/src/box/lua/tuple.c
>> @@ -283,16 +283,14 @@ lbox_tuple_to_map(struct lua_State *L)
>>      int argc = lua_gettop(L);
>>      if (argc < 1 || argc > 2)
>>          goto error;
>> -    int names_only = 0;
>> +    bool names_only = 0;
>>      if (argc == 2) {
>>> 
>>>> +    if (argc == 2) {
>>>> +        if (!lua_istable(L, 2))
>>>> +            goto error;
>>>> +        lua_getfield(L, 2, "names_only");
>>>> +        if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) {
>>>> +            goto error;
>>>> +        }
>>> 
>>> 2. We put 'if' body in {} only if it consists of multiple lines. For single line
>>> do not use {}.
>>          if (!lua_istable(L, 2))
>>              goto error;
>>          lua_getfield(L, 2, "names_only");
>> -        if (!lua_isboolean(L, -1) && !lua_isnil(L, -1)) {
>> +        if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
>>              goto error;
>> -        }
>>> 
>>>> +        names_only = lua_toboolean(L, -1);
>>>> +        lua_pop(L, 1);
>>> 
>>> 3. As I already wrote, you should not pop temporary values from the stack -
>>> Lua does it for you.
>>> 
>>          names_only = lua_toboolean(L, -1);
>> -        lua_pop(L, 1);
>>      }
>>      const struct tuple *tuple = lua_checktuple(L, 1);
>> @@ -327,7 +325,7 @@ lbox_tuple_to_map(struct lua_State *L)
>>      }
>>      return 1;
>>  error:
>> -    luaL_error(L, "Usage: tuple:tomap(params)");
>> +    luaL_error(L, "Usage: tuple:tomap(opts)");
>>      return 1;
>>  }
>>>> +    }
>>>> +
>>>>     const struct tuple *tuple = lua_checktuple(L, 1);
>>>>     const struct tuple_format *format = tuple_format(tuple);
>>>>     const struct tuple_field *field = &format->fields[0];
>>>> @@ -294,6 +307,8 @@ lbox_tuple_to_map(struct lua_State *L)
>>>>         lua_pushstring(L, field->name);
>>>>         luamp_decode(L, luaL_msgpack_default, &pos);
>>>>         lua_rawset(L, -3);
>>>> +        if (names_only)
>>>> +            continue;
>>>>         /*
>>>>          * Access the same field by an index. There is no
>>>>          * copy for tables - lua optimizes it and uses
>>>> @@ -303,12 +318,17 @@ lbox_tuple_to_map(struct lua_State *L)
>>>>         lua_rawget(L, -2);
>>>>         lua_rawseti(L, -2, i + TUPLE_INDEX_BASE);
>>>>     }
>>>> +    if (names_only)
>>>> +        return 1;
>>>>     /* Access for not named fields by index. */
>>>>     for (int i = n_named; i < field_count; ++i) {
>>>>         luamp_decode(L, luaL_msgpack_default, &pos);
>>>>         lua_rawseti(L, -2, i + TUPLE_INDEX_BASE);
>>>>     }
>>>>     return 1;
>>>> +error:
>>>> +    luaL_error(L, "Usage: tuple:tomap(params)");
>>> 
>>> 4. I asked you to write 'opts', not 'params'. Tomap accept not multiple
>>> parameters, but single opts map.
>>> 
>>>> +    return 1;
>>>> }
>>>> 
>>>> /**
>>>> diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
>>>> index 6d7d254..3411947 100644
>>>> --- a/test/engine/tuple.test.lua
>>>> +++ b/test/engine/tuple.test.lua
>>>> @@ -200,5 +200,21 @@ t1map = t1:tomap()
>>>> maplen(t1map), t1map[1], t1map[2], t1map[3]
>>>> s:drop()
>>>> 
>>>> +--
>>>> +-- gh-2821: tuple:tomap() names_only feature.
>>>> +--
>>>> +format = {}
>>>> +format[1] = {name = 'field1', type = 'unsigned' }
>>>> +format[2] = {name = 'field2', type = 'unsigned' }
>>>> +s = box.schema.create_space('test', {format = format})
>>>> +pk = s:create_index('pk')
>>>> +t = s:replace{100, 200, 300 }
>>>> +t:tomap({names_only = false})
>>>> +t:tomap({names_only = true})
>>>> +t:tomap({names_only = 'text'})
>>>> +t:tomap({names_only = true}, {dummy = true})
>>>> +t:tomap({})
>>>> +s:drop()
>>> 
>>> 5. What if I create a space with no format, and will call
>>> tomap({names_only = true}) on its tuple?
>>> 
>>>> +
>>>> engine = nil
>>>> test_run = nil
>>>> -- 
>>>> 2.7.4
>>>> 
>>>> 
>>> 
>>> 
>> diff --git a/test/engine/tuple.result b/test/engine/tuple.result
>> index f9805b0..9fd9bdc 100644
>> --- a/test/engine/tuple.result
>> +++ b/test/engine/tuple.result
>> @@ -626,11 +626,11 @@ t:tomap({names_only = true})
>>  ...
>>  t:tomap({names_only = 'text'})
>>  ---
>> -- error: 'Usage: tuple:tomap() or tuple:tomap({names_only = true})'
>> +- error: 'Usage: tuple:tomap(opts)'
>>  ...
>>  t:tomap({names_only = true}, {dummy = true})
>>  ---
>> -- error: 'Usage: tuple:tomap() or tuple:tomap({names_only = true})'
>> +- error: 'Usage: tuple:tomap(opts)'
>>  ...
>>  t:tomap({})
>>  ---
>> @@ -643,6 +643,22 @@ t:tomap({})
>>  s:drop()
>>  ---
>>  ...
>> +s = box.schema.create_space('test')
>> +---
>> +...
>> +pk = s:create_index('pk')
>> +---
>> +...
>> +t = s:replace{1,2,3,4,5,6,7}
>> +---
>> +...
>> +t:tomap({names_only = true})
>> +---
>> +- []
>> +...
>> +s:drop()
>> +---
>> +...
>>  engine = nil
>>  ---
>>  ...
>> diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
>> index 3411947..0af93ca 100644
>> --- a/test/engine/tuple.test.lua
>> +++ b/test/engine/tuple.test.lua
>> @@ -215,6 +215,11 @@ t:tomap({names_only = 'text'})
>>  t:tomap({names_only = true}, {dummy = true})
>>  t:tomap({})
>>  s:drop()
>> +s = box.schema.create_space('test')
>> +pk = s:create_index('pk')
>> +t = s:replace{1,2,3,4,5,6,7}
>> +t:tomap({names_only = true})
>> +s:drop()
>>  engine = nil
>>  test_run = nil
>