* [tarantool-patches] [PATCH v2 1/1] tuple:tomap: only_names option @ 2018-03-27 10:06 Kirill Shcherbatov 2018-03-27 10:14 ` [tarantool-patches] " v.shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Kirill Shcherbatov @ 2018-03-27 10:06 UTC (permalink / raw) To: tarantool-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option 2018-03-27 10:06 [tarantool-patches] [PATCH v2 1/1] tuple:tomap: only_names option Kirill Shcherbatov @ 2018-03-27 10:14 ` v.shpilevoy 2018-03-27 10:28 ` Kirill Shcherbatov 0 siblings, 1 reply; 8+ messages in thread From: v.shpilevoy @ 2018-03-27 10:14 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Shcherbatov 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option 2018-03-27 10:14 ` [tarantool-patches] " v.shpilevoy @ 2018-03-27 10:28 ` Kirill Shcherbatov 0 siblings, 0 replies; 8+ messages in thread From: Kirill Shcherbatov @ 2018-03-27 10:28 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches 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 >> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <b9c53caf-7c00-6bef-3e90-c71f519553fe@tarantool.org>]
* [tarantool-patches] [PATCH v2 1/1] tuple:tomap: only_names option [not found] <b9c53caf-7c00-6bef-3e90-c71f519553fe@tarantool.org> @ 2018-03-27 15:00 ` Kirill Shcherbatov 2018-03-27 15:34 ` [tarantool-patches] " v.shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Kirill Shcherbatov @ 2018-03-27 15:00 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Shcherbatov 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; + 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; + } + 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]; @@ -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)"); + return 1; } /** diff --git a/test/engine/tuple.result b/test/engine/tuple.result index b3b23b2..f9805b0 100644 --- a/test/engine/tuple.result +++ b/test/engine/tuple.result @@ -590,6 +590,59 @@ 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}) +--- +- 1: 100 + 2: 200 + 3: 300 + field1: 100 + field2: 200 +... +t:tomap({names_only = true}) +--- +- field1: 100 + field2: 200 +... +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})' +... +t:tomap({}) +--- +- 1: 100 + 2: 200 + 3: 300 + field1: 100 + field2: 200 +... +s:drop() +--- +... engine = nil --- ... 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() + engine = nil test_run = nil -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option 2018-03-27 15:00 ` [tarantool-patches] " Kirill Shcherbatov @ 2018-03-27 15:34 ` v.shpilevoy 2018-03-28 8:01 ` Kirill Shcherbatov 0 siblings, 1 reply; 8+ messages in thread From: v.shpilevoy @ 2018-03-27 15:34 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Shcherbatov 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 > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option 2018-03-27 15:34 ` [tarantool-patches] " v.shpilevoy @ 2018-03-28 8:01 ` Kirill Shcherbatov 2018-03-28 8:05 ` Kirill Shcherbatov 0 siblings, 1 reply; 8+ messages in thread From: Kirill Shcherbatov @ 2018-03-28 8:01 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy 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 -- ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option 2018-03-28 8:01 ` Kirill Shcherbatov @ 2018-03-28 8:05 ` Kirill Shcherbatov 2018-03-28 8:16 ` v.shpilevoy 0 siblings, 1 reply; 8+ messages in thread From: Kirill Shcherbatov @ 2018-03-28 8:05 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy - 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option 2018-03-28 8:05 ` Kirill Shcherbatov @ 2018-03-28 8:16 ` v.shpilevoy 2018-03-28 10:20 ` Vladimir Davydov 0 siblings, 1 reply; 8+ messages in thread From: v.shpilevoy @ 2018-03-28 8:16 UTC (permalink / raw) To: tarantool-patches; +Cc: Vladimir Davydov 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 1/1] tuple:tomap: only_names option 2018-03-28 8:16 ` v.shpilevoy @ 2018-03-28 10:20 ` Vladimir Davydov 0 siblings, 0 replies; 8+ messages in thread From: Vladimir Davydov @ 2018-03-28 10:20 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: v.shpilevoy, tarantool-patches 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(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-03-28 10:20 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-27 10:06 [tarantool-patches] [PATCH v2 1/1] tuple:tomap: only_names option Kirill Shcherbatov
2018-03-27 10:14 ` [tarantool-patches] " v.shpilevoy
2018-03-27 10:28 ` Kirill Shcherbatov
[not found] <b9c53caf-7c00-6bef-3e90-c71f519553fe@tarantool.org>
2018-03-27 15:00 ` [tarantool-patches] " Kirill Shcherbatov
2018-03-27 15:34 ` [tarantool-patches] " v.shpilevoy
2018-03-28 8:01 ` Kirill Shcherbatov
2018-03-28 8:05 ` Kirill Shcherbatov
2018-03-28 8:16 ` v.shpilevoy
2018-03-28 10:20 ` Vladimir Davydov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox