From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5FA7F2A6F8 for ; Tue, 27 Mar 2018 06:28:15 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id vCpdwoOD8031 for ; Tue, 27 Mar 2018 06:28:15 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1B6B529927 for ; Tue, 27 Mar 2018 06:28:15 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option References: <9131f9838c0bed9b459a974ed07ffd0a8817ca88.1522145167.git.kshcherbatov@tarantool.org> <2169D9B3-D807-4BF1-9996-EB5E60B70BFB@tarantool.org> From: Kirill Shcherbatov Message-ID: <569d3847-20b4-53f7-c81b-358cff67ffcc@tarantool.org> Date: Tue, 27 Mar 2018 13:28:13 +0300 MIME-Version: 1.0 In-Reply-To: <2169D9B3-D807-4BF1-9996-EB5E60B70BFB@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "v.shpilevoy@tarantool.org" Cc: tarantool-patches@freelists.org 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 написал(а): >> >> 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 >> >> >