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

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

[Kirill Shcherbatov]

Fixes #3280
---
 src/box/lua/tuple.c        | 23 ++++++++++++++++++++---
 test/engine/tuple.result   | 45 +++++++++++++++++++++++++++++++++++++++++++++
 test/engine/tuple.test.lua | 14 ++++++++++++++
 3 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index d5ed7c4..da2a80b 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -280,9 +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()");
-	const struct tuple *tuple = lua_checktuple(L, 1);
+    int argc = lua_gettop(L);
+	if (argc < 1 || argc > 2)
+		goto error;
+	int names_only = 0;
+	if (argc == 2) {
+		if (!lua_istable(L, 2))
+			goto error;
+		lua_getfield(L, 2, "names_only");
+		if (!lua_isboolean(L, -1))
+			goto error;
+		names_only = lua_toboolean(L, -1);
+		lua_pop(L, 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];
 	const char *pos = tuple_data(tuple);
@@ -294,6 +306,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
@@ -309,6 +323,9 @@ lbox_tuple_to_map(struct lua_State *L)
 		lua_rawseti(L, -2, i + TUPLE_INDEX_BASE);
 	}
 	return 1;
+error:
+	luaL_error(L, "Usage: tuple:tomap() or tuple:tomap({names_only = true})");
+	return 1;
 }
 
 /**
diff --git a/test/engine/tuple.result b/test/engine/tuple.result
index b3b23b2..a8ba086 100644
--- a/test/engine/tuple.result
+++ b/test/engine/tuple.result
@@ -590,6 +590,51 @@ maplen(t1map), t1map[1], t1map[2], t1map[3]
 s:drop()
 ---
 ...
+--
+-- gh-2821: tuple:tomap() names_only feature.
+--
+subscriber = box.schema.space.create('subscriber')
+---
+...
+box.schema.user.grant('guest','read,write,execute','space', 'subscriber')
+---
+...
+subscriber:format({{'contract_id', 'integer'}, {'subscriber_id', 'integer'}, {'phone', 'string'}, {'subscriber_status_id', 'integer'}, })
+---
+...
+idx = subscriber:create_index('primary', {parts={'contract_id', 'subscriber_id'}})
+---
+...
+--subscriber:create_index('phone', {parts={'phone'}, unique = false })
+subscriber:replace({1, 1, '79160000000', 42})
+---
+- [1, 1, '79160000000', 42]
+...
+t = subscriber:get({1, 1})
+---
+...
+t:tomap({names_only = false})
+---
+- 1: 1
+  2: 1
+  3: '79160000000'
+  4: 42
+  subscriber_status_id: 42
+  contract_id: 1
+  phone: '79160000000'
+  subscriber_id: 1
+...
+t:tomap({names_only = true})
+---
+- subscriber_status_id: 42
+  contract_id: 1
+  phone: '79160000000'
+  subscriber_id: 1
+...
+s:drop()
+---
+- error: Space 'test' does not exist
+...
 engine = nil
 ---
 ...
diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
index 6d7d254..0f139a3 100644
--- a/test/engine/tuple.test.lua
+++ b/test/engine/tuple.test.lua
@@ -200,5 +200,19 @@ t1map = t1:tomap()
 maplen(t1map), t1map[1], t1map[2], t1map[3]
 s:drop()
 
+--
+-- gh-2821: tuple:tomap() names_only feature.
+--
+subscriber = box.schema.space.create('subscriber')
+box.schema.user.grant('guest','read,write,execute','space', 'subscriber')
+subscriber:format({{'contract_id', 'integer'}, {'subscriber_id', 'integer'}, {'phone', 'string'}, {'subscriber_status_id', 'integer'}, })
+idx = subscriber:create_index('primary', {parts={'contract_id', 'subscriber_id'}})
+--subscriber:create_index('phone', {parts={'phone'}, unique = false })
+subscriber:replace({1, 1, '79160000000', 42})
+t = subscriber:get({1, 1})
+t:tomap({names_only = false})
+t:tomap({names_only = true})
+s:drop()
+
 engine = nil
 test_run = nil
-- 
2.7.4

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

[v.shpilevoy]

See below 5 comments.

> 27 марта 2018 г., в 13:06, Kirill Shcherbatov <kshcherbatov@tarantool.org> написал(а):
> 
> Fixes #3280
> ---
> src/box/lua/tuple.c        | 23 ++++++++++++++++++++---
> test/engine/tuple.result   | 45 +++++++++++++++++++++++++++++++++++++++++++++
> test/engine/tuple.test.lua | 14 ++++++++++++++
> 3 files changed, 79 insertions(+), 3 deletions(-)
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index d5ed7c4..da2a80b 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -280,9 +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()");
> -	const struct tuple *tuple = lua_checktuple(L, 1);
> +    int argc = lua_gettop(L);
> +	if (argc < 1 || argc > 2)
> +		goto error;
> +	int names_only = 0;
> +	if (argc == 2) {
> +		if (!lua_istable(L, 2))
> +			goto error;
> +		lua_getfield(L, 2, "names_only");
> +		if (!lua_isboolean(L, -1))
> +			goto error;
> +		names_only = lua_toboolean(L, -1);
> +		lua_pop(L, 1);

1. Seems that you fixed only indentation. Please, fix other comments too.

> +    }
> +
> +    const struct tuple *tuple = lua_checktuple(L, 1);
> 	const struct tuple_format *format = tuple_format(tuple);
> 	const struct tuple_field *field = &format->fields[0];
> 	const char *pos = tuple_data(tuple);
> @@ -294,6 +306,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
> @@ -309,6 +323,9 @@ lbox_tuple_to_map(struct lua_State *L)
> 		lua_rawseti(L, -2, i + TUPLE_INDEX_BASE);
> 	}
> 	return 1;
> +error:
> +	luaL_error(L, "Usage: tuple:tomap() or tuple:tomap({names_only = true})");
> +	return 1;
> }
> 
> /**
> 
> diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
> index 6d7d254..0f139a3 100644
> --- a/test/engine/tuple.test.lua
> +++ b/test/engine/tuple.test.lua
> @@ -200,5 +200,19 @@ t1map = t1:tomap()
> maplen(t1map), t1map[1], t1map[2], t1map[3]
> s:drop()
> 
> +--
> +-- gh-2821: tuple:tomap() names_only feature.
> +--
> +subscriber = box.schema.space.create('subscriber')
> +box.schema.user.grant('guest','read,write,execute','space', 'subscriber')

2. You do not need rights, when work locally.

> +subscriber:format({{'contract_id', 'integer'}, {'subscriber_id', 'integer'}, {'phone', 'string'}, {'subscriber_status_id', 'integer'}, })
> +idx = subscriber:create_index('primary', {parts={'contract_id', 'subscriber_id'}})
> +--subscriber:create_index('phone', {parts={'phone'}, unique = false })

3. Please, do not put a code in comment. If you do not need it, then just delete.

> +subscriber:replace({1, 1, '79160000000', 42})
> +t = subscriber:get({1, 1})

4. replace() returns a result tuple, so you can just do t = subscriber:replace(), with no get().

> +t:tomap({names_only = false})
> +t:tomap({names_only = true})

5. Like I wrote in the previous review, please, add tests on tomap with invalid options, on tomap with names_only = non-boolean, on tomap with empty options. Do not hurry so much.

> +s:drop()
> +
> engine = nil
> test_run = nil
> -- 
> 2.7.4
> 
> 

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

[Kirill Shcherbatov]

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index da2a80b..dc4063b 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -280,7 +280,7 @@ luamp_encode_extension_box(struct lua_State *L, int 
idx,
  static int
  lbox_tuple_to_map(struct lua_State *L)
  {
-    int argc = lua_gettop(L);
+    int argc = lua_gettop(L);
      if (argc < 1 || argc > 2)
          goto error;
      int names_only = 0;
diff --git a/test/engine/tuple.result b/test/engine/tuple.result
index a8ba086..060169b 100644
--- a/test/engine/tuple.result
+++ b/test/engine/tuple.result
@@ -596,21 +596,13 @@ s:drop()
  subscriber = box.schema.space.create('subscriber')
  ---
  ...
-box.schema.user.grant('guest','read,write,execute','space', 'subscriber')
----
-...
  subscriber:format({{'contract_id', 'integer'}, {'subscriber_id', 
'integer'}, {'phone', 'string'}, {'subscriber_status_id', 'integer'}, })
  ---
  ...
  idx = subscriber:create_index('primary', {parts={'contract_id', 
'subscriber_id'}})
  ---
  ...
---subscriber:create_index('phone', {parts={'phone'}, unique = false })
-subscriber:replace({1, 1, '79160000000', 42})
----
-- [1, 1, '79160000000', 42]
-...
-t = subscriber:get({1, 1})
+t = subscriber:replace({1, 1, '79160000000', 42})
  ---
  ...
  t:tomap({names_only = false})
@@ -631,6 +623,14 @@ t:tomap({names_only = true})
    phone: '79160000000'
    subscriber_id: 1
  ...
+t:tomap({names_only = 'text'})
+---
+- error: 'Usage: tuple:tomap() or tuple:tomap({names_only = true})'
+...
+t:tomap({names_only = true}, {dummy = true})
+---
+- error: 'Usage: tuple:tomap() or tuple:tomap({names_only = true})'
+...
  s:drop()
  ---
  - error: Space 'test' does not exist
diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
index 0f139a3..e4daca7 100644
--- a/test/engine/tuple.test.lua
+++ b/test/engine/tuple.test.lua
@@ -204,14 +204,13 @@ s:drop()
  -- gh-2821: tuple:tomap() names_only feature.
  --
  subscriber = box.schema.space.create('subscriber')
-box.schema.user.grant('guest','read,write,execute','space', 'subscriber')
  subscriber:format({{'contract_id', 'integer'}, {'subscriber_id', 
'integer'}, {'phone', 'string'}, {'subscriber_status_id', 'integer'}, })
  idx = subscriber:create_index('primary', {parts={'contract_id', 
'subscriber_id'}})
---subscriber:create_index('phone', {parts={'phone'}, unique = false })
-subscriber:replace({1, 1, '79160000000', 42})
-t = subscriber:get({1, 1})
+t = subscriber:replace({1, 1, '79160000000', 42})
  t:tomap({names_only = false})
  t:tomap({names_only = true})
+t:tomap({names_only = 'text'})
+t:tomap({names_only = true}, {dummy = true})
  s:drop()

  engine = nil


On 27.03.2018 13:14, v.shpilevoy@tarantool.org wrote:
> See below 5 comments.
>
>> 27 марта 2018 г., в 13:06, Kirill Shcherbatov <kshcherbatov@tarantool.org> написал(а):
>>
>> Fixes #3280
>> ---
>> src/box/lua/tuple.c        | 23 ++++++++++++++++++++---
>> test/engine/tuple.result   | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> test/engine/tuple.test.lua | 14 ++++++++++++++
>> 3 files changed, 79 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
>> index d5ed7c4..da2a80b 100644
>> --- a/src/box/lua/tuple.c
>> +++ b/src/box/lua/tuple.c
>> @@ -280,9 +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()");
>> -	const struct tuple *tuple = lua_checktuple(L, 1);
>> +    int argc = lua_gettop(L);
>> +	if (argc < 1 || argc > 2)
>> +		goto error;
>> +	int names_only = 0;
>> +	if (argc == 2) {
>> +		if (!lua_istable(L, 2))
>> +			goto error;
>> +		lua_getfield(L, 2, "names_only");
>> +		if (!lua_isboolean(L, -1))
>> +			goto error;
>> +		names_only = lua_toboolean(L, -1);
>> +		lua_pop(L, 1);
> 1. Seems that you fixed only indentation. Please, fix other comments too.
>
>> +    }
>> +
>> +    const struct tuple *tuple = lua_checktuple(L, 1);
>> 	const struct tuple_format *format = tuple_format(tuple);
>> 	const struct tuple_field *field = &format->fields[0];
>> 	const char *pos = tuple_data(tuple);
>> @@ -294,6 +306,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
>> @@ -309,6 +323,9 @@ lbox_tuple_to_map(struct lua_State *L)
>> 		lua_rawseti(L, -2, i + TUPLE_INDEX_BASE);
>> 	}
>> 	return 1;
>> +error:
>> +	luaL_error(L, "Usage: tuple:tomap() or tuple:tomap({names_only = true})");
>> +	return 1;
>> }
>>
>> /**
>>
>> diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
>> index 6d7d254..0f139a3 100644
>> --- a/test/engine/tuple.test.lua
>> +++ b/test/engine/tuple.test.lua
>> @@ -200,5 +200,19 @@ t1map = t1:tomap()
>> maplen(t1map), t1map[1], t1map[2], t1map[3]
>> s:drop()
>>
>> +--
>> +-- gh-2821: tuple:tomap() names_only feature.
>> +--
>> +subscriber = box.schema.space.create('subscriber')
>> +box.schema.user.grant('guest','read,write,execute','space', 'subscriber')
> 2. You do not need rights, when work locally.
>
>> +subscriber:format({{'contract_id', 'integer'}, {'subscriber_id', 'integer'}, {'phone', 'string'}, {'subscriber_status_id', 'integer'}, })
>> +idx = subscriber:create_index('primary', {parts={'contract_id', 'subscriber_id'}})
>> +--subscriber:create_index('phone', {parts={'phone'}, unique = false })
> 3. Please, do not put a code in comment. If you do not need it, then just delete.
>
>> +subscriber:replace({1, 1, '79160000000', 42})
>> +t = subscriber:get({1, 1})
> 4. replace() returns a result tuple, so you can just do t = subscriber:replace(), with no get().
>
>> +t:tomap({names_only = false})
>> +t:tomap({names_only = true})
> 5. Like I wrote in the previous review, please, add tests on tomap with invalid options, on tomap with names_only = non-boolean, on tomap with empty options. Do not hurry so much.
>
>> +s:drop()
>> +
>> engine = nil
>> test_run = nil
>> -- 
>> 2.7.4
>>
>>
>

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

[Vladimir Davydov]

Ack, but please in future add more descriptive comments to your patches.
In this particular case it would be nice to have a description of the
new option in the commit log (what it does, an example of usage).

> From 14a62b394bf7b21d6c9549875abce5c71416c0b3 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Wed, 28 Mar 2018 11:05:19 +0300
> Subject: [PATCH] tuple:tomap: only_names option
> 
> Fixes #3280
> 
>  src/box/lua/tuple.c        | 22 +++++++++++++--
>  test/engine/tuple.result   | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  test/engine/tuple.test.lua | 21 ++++++++++++++
>  3 files changed, 110 insertions(+), 2 deletions(-)

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

[v.shpilevoy]

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
> 

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

[Kirill Shcherbatov]

-       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

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

[Kirill Shcherbatov]

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
-- 

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

[v.shpilevoy]

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.

> +	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 {}.

> +		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.

> +	}
> +
> 	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
> 
>