[tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue Mar 27 13:28:13 MSK 2018
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 at tarantool.org wrote:
> See below 5 comments.
>
>> 27 марта 2018 г., в 13:06, Kirill Shcherbatov <kshcherbatov at 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
>>
>>
>
More information about the Tarantool-patches
mailing list