* [tarantool-patches] [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields @ 2018-08-22 10:43 imeevma 2018-08-22 14:16 ` [tarantool-patches] " Vladislav Shpilevoy 0 siblings, 1 reply; 4+ messages in thread From: imeevma @ 2018-08-22 10:43 UTC (permalink / raw) To: tarantool-patches; +Cc: v.shpilevoy Tuple method 'tomap' in some cases worked impropertly if tuple length less than it should be according to space format. Fixed in this patch. Closes #3631 --- Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3631-wrong-tomap-work-with-nullable-fields Issue: https://github.com/tarantool/tarantool/issues/3631 src/box/lua/tuple.c | 4 +-- test/engine/tuple.result | 63 ++++++++++++++++++++++++++++++++++++++++++++++ test/engine/tuple.test.lua | 23 +++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c index 7ca4299..856042c 100644 --- a/src/box/lua/tuple.c +++ b/src/box/lua/tuple.c @@ -284,12 +284,12 @@ lbox_tuple_to_map(struct lua_State *L) luaL_error(L, "Usage: tuple:tomap()"); 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); int field_count = (int)mp_decode_array(&pos); int n_named = format->dict->name_count; lua_createtable(L, field_count, n_named); - for (int i = 0; i < n_named; ++i, ++field) { + int fields_number = n_named > field_count ? field_count : n_named; + for (int i = 0; i < fields_number; ++i) { /* Access by name. */ const char *name = format->dict->names[i]; lua_pushstring(L, name); diff --git a/test/engine/tuple.result b/test/engine/tuple.result index b3b23b2..b9f42b9 100644 --- a/test/engine/tuple.result +++ b/test/engine/tuple.result @@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3] s:drop() --- ... +-- +-- gh-3631: Wrong 'tomap' work with nullable fields +-- +format = {} +--- +... +format[1] = {'first', 'unsigned'} +--- +... +format[2] = {'second', 'unsigned'} +--- +... +format[3] = {'third', 'unsigned'} +--- +... +format[4] = {'fourth', 'string', is_nullable = true} +--- +... +s = box.schema.space.create('test', {format = format, engine = engine}) +--- +... +pk = s:create_index('primary', {parts = { 1, 'unsigned'}}) +--- +... +s:insert({1, 2, 3}) +--- +- [1, 2, 3] +... +tuple = s:get(1) +--- +... +tuple +--- +- [1, 2, 3] +... +-- Should be NULL +tuple.fourth +--- +- null +... +-- Should have only three named fields +tuple:tomap() +--- +- 1: 1 + 2: 2 + 3: 3 + third: 3 + second: 2 + first: 1 +... +-- Should be NULL +tuple:tomap().fourth +--- +- null +... +-- Should be nil +type(tuple:tomap().fourth) +--- +- nil +... +s:drop() +--- +... engine = nil --- ... diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua index 6d7d254..10b888e 100644 --- a/test/engine/tuple.test.lua +++ b/test/engine/tuple.test.lua @@ -200,5 +200,28 @@ t1map = t1:tomap() maplen(t1map), t1map[1], t1map[2], t1map[3] s:drop() +-- +-- gh-3631: Wrong 'tomap' work with nullable fields +-- +format = {} +format[1] = {'first', 'unsigned'} +format[2] = {'second', 'unsigned'} +format[3] = {'third', 'unsigned'} +format[4] = {'fourth', 'string', is_nullable = true} +s = box.schema.space.create('test', {format = format, engine = engine}) +pk = s:create_index('primary', {parts = { 1, 'unsigned'}}) +s:insert({1, 2, 3}) +tuple = s:get(1) +tuple +-- Should be NULL +tuple.fourth +-- Should have only three named fields +tuple:tomap() +-- Should be NULL +tuple:tomap().fourth +-- Should be nil +type(tuple:tomap().fourth) +s:drop() + engine = nil test_run = nil -- 2.7.4 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields 2018-08-22 10:43 [tarantool-patches] [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields imeevma @ 2018-08-22 14:16 ` Vladislav Shpilevoy 2018-08-22 15:31 ` Imeev Mergen 0 siblings, 1 reply; 4+ messages in thread From: Vladislav Shpilevoy @ 2018-08-22 14:16 UTC (permalink / raw) To: imeevma, tarantool-patches Hi! Thanks for the patch! See 3 comments below. On 22/08/2018 13:43, imeevma@tarantool.org wrote: > Tuple method 'tomap' in some cases worked impropertly if tuple 1. Typo: 'impropertly'. > length less than it should be according to space format. Fixed > in this patch. > > Closes #3631 > --- > Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3631-wrong-tomap-work-with-nullable-fields > Issue: https://github.com/tarantool/tarantool/issues/3631 > > src/box/lua/tuple.c | 4 +-- > test/engine/tuple.result | 63 ++++++++++++++++++++++++++++++++++++++++++++++ > test/engine/tuple.test.lua | 23 +++++++++++++++++ > 3 files changed, 88 insertions(+), 2 deletions(-) > > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c > index 7ca4299..856042c 100644 > --- a/src/box/lua/tuple.c > +++ b/src/box/lua/tuple.c > @@ -284,12 +284,12 @@ lbox_tuple_to_map(struct lua_State *L) > luaL_error(L, "Usage: tuple:tomap()"); > 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); > int field_count = (int)mp_decode_array(&pos); > int n_named = format->dict->name_count; > lua_createtable(L, field_count, n_named); > - for (int i = 0; i < n_named; ++i, ++field) { > + int fields_number = n_named > field_count ? field_count : n_named; 2. Please, use MIN macros. It is available in this file as I remember. > + for (int i = 0; i < fields_number; ++i) { > /* Access by name. */ > const char *name = format->dict->names[i]; > lua_pushstring(L, name); > diff --git a/test/engine/tuple.result b/test/engine/tuple.result > index b3b23b2..b9f42b9 100644 > --- a/test/engine/tuple.result > +++ b/test/engine/tuple.result > @@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3] > s:drop() > --- > ... > +-- > +-- gh-3631: Wrong 'tomap' work with nullable fields > +-- > +format = {} > +--- > +... > +format[1] = {'first', 'unsigned'} > +--- > +... > +format[2] = {'second', 'unsigned'} > +--- > +... > +format[3] = {'third', 'unsigned'} > +--- > +... > +format[4] = {'fourth', 'string', is_nullable = true} > +--- > +... > +s = box.schema.space.create('test', {format = format, engine = engine}) > +--- > +... > +pk = s:create_index('primary', {parts = { 1, 'unsigned'}}) 3. Odd white space before '1'. > +--- > +... > +s:insert({1, 2, 3}) > +--- > +- [1, 2, 3] > +... > +tuple = s:get(1) > +--- > +... ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields 2018-08-22 14:16 ` [tarantool-patches] " Vladislav Shpilevoy @ 2018-08-22 15:31 ` Imeev Mergen 2018-08-24 10:23 ` Vladislav Shpilevoy 0 siblings, 1 reply; 4+ messages in thread From: Imeev Mergen @ 2018-08-22 15:31 UTC (permalink / raw) To: Vladislav Shpilevoy, tarantool-patches Hello! Thank you for the review! On 08/22/2018 05:16 PM, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! See 3 comments below. > > On 22/08/2018 13:43, imeevma@tarantool.org wrote: >> Tuple method 'tomap' in some cases worked impropertly if tuple > > 1. Typo: 'impropertly'. Fixed. > >> length less than it should be according to space format. Fixed >> in this patch. >> >> Closes #3631 >> --- >> Branch: >> https://github.com/tarantool/tarantool/tree/imeevma/gh-3631-wrong-tomap-work-with-nullable-fields >> Issue: https://github.com/tarantool/tarantool/issues/3631 >> >> src/box/lua/tuple.c | 4 +-- >> test/engine/tuple.result | 63 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> test/engine/tuple.test.lua | 23 +++++++++++++++++ >> 3 files changed, 88 insertions(+), 2 deletions(-) >> >> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c >> index 7ca4299..856042c 100644 >> --- a/src/box/lua/tuple.c >> +++ b/src/box/lua/tuple.c >> @@ -284,12 +284,12 @@ lbox_tuple_to_map(struct lua_State *L) >> luaL_error(L, "Usage: tuple:tomap()"); >> 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); >> int field_count = (int)mp_decode_array(&pos); >> int n_named = format->dict->name_count; >> lua_createtable(L, field_count, n_named); >> - for (int i = 0; i < n_named; ++i, ++field) { >> + int fields_number = n_named > field_count ? field_count : n_named; > > 2. Please, use MIN macros. It is available in this file as I > remember. Fixed. > >> + for (int i = 0; i < fields_number; ++i) { >> /* Access by name. */ >> const char *name = format->dict->names[i]; >> lua_pushstring(L, name); >> diff --git a/test/engine/tuple.result b/test/engine/tuple.result >> index b3b23b2..b9f42b9 100644 >> --- a/test/engine/tuple.result >> +++ b/test/engine/tuple.result >> @@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3] >> s:drop() >> --- >> ... >> +-- >> +-- gh-3631: Wrong 'tomap' work with nullable fields >> +-- >> +format = {} >> +--- >> +... >> +format[1] = {'first', 'unsigned'} >> +--- >> +... >> +format[2] = {'second', 'unsigned'} >> +--- >> +... >> +format[3] = {'third', 'unsigned'} >> +--- >> +... >> +format[4] = {'fourth', 'string', is_nullable = true} >> +--- >> +... >> +s = box.schema.space.create('test', {format = format, engine = engine}) >> +--- >> +... >> +pk = s:create_index('primary', {parts = { 1, 'unsigned'}}) > > 3. Odd white space before '1'. Fixed. > >> +--- >> +... >> +s:insert({1, 2, 3}) >> +--- >> +- [1, 2, 3] >> +... >> +tuple = s:get(1) >> +--- >> +... commit 066db38393316c486b9edef0d71e854c3e4fdc78 Author: Mergen Imeev <imeevma@gmail.com> Date: Wed Aug 22 13:33:22 2018 +0300 lua: wrong 'tomap' work with nullable fields Tuple method 'tomap' in some cases worked improperly if tuple length less than it should be according to space format. Fixed in this patch. Closes #3631 diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c index 7ca4299..cd96152 100644 --- a/src/box/lua/tuple.c +++ b/src/box/lua/tuple.c @@ -284,12 +284,11 @@ lbox_tuple_to_map(struct lua_State *L) luaL_error(L, "Usage: tuple:tomap()"); 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); int field_count = (int)mp_decode_array(&pos); int n_named = format->dict->name_count; lua_createtable(L, field_count, n_named); - for (int i = 0; i < n_named; ++i, ++field) { + for (int i = 0; i < MIN(field_count, n_named); ++i) { /* Access by name. */ const char *name = format->dict->names[i]; lua_pushstring(L, name); diff --git a/test/engine/tuple.result b/test/engine/tuple.result index b3b23b2..88bb3b4 100644 --- a/test/engine/tuple.result +++ b/test/engine/tuple.result @@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3] s:drop() --- ... +-- +-- gh-3631: Wrong 'tomap' work with nullable fields +-- +format = {} +--- +... +format[1] = {'first', 'unsigned'} +--- +... +format[2] = {'second', 'unsigned'} +--- +... +format[3] = {'third', 'unsigned'} +--- +... +format[4] = {'fourth', 'string', is_nullable = true} +--- +... +s = box.schema.space.create('test', {format = format, engine = engine}) +--- +... +pk = s:create_index('primary', {parts = {1, 'unsigned'}}) +--- +... +s:insert({1, 2, 3}) +--- +- [1, 2, 3] +... +tuple = s:get(1) +--- +... +tuple +--- +- [1, 2, 3] +... +-- Should be NULL +tuple.fourth +--- +- null +... +-- Should have only three named fields +tuple:tomap() +--- +- 1: 1 + 2: 2 + 3: 3 + third: 3 + second: 2 + first: 1 +... +-- Should be NULL +tuple:tomap().fourth +--- +- null +... +-- Should be nil +type(tuple:tomap().fourth) +--- +- nil +... +s:drop() +--- +... engine = nil --- ... diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua index 6d7d254..723dde4 100644 --- a/test/engine/tuple.test.lua +++ b/test/engine/tuple.test.lua @@ -200,5 +200,28 @@ t1map = t1:tomap() maplen(t1map), t1map[1], t1map[2], t1map[3] s:drop() +-- +-- gh-3631: Wrong 'tomap' work with nullable fields +-- +format = {} +format[1] = {'first', 'unsigned'} +format[2] = {'second', 'unsigned'} +format[3] = {'third', 'unsigned'} +format[4] = {'fourth', 'string', is_nullable = true} +s = box.schema.space.create('test', {format = format, engine = engine}) +pk = s:create_index('primary', {parts = {1, 'unsigned'}}) +s:insert({1, 2, 3}) +tuple = s:get(1) +tuple +-- Should be NULL +tuple.fourth +-- Should have only three named fields +tuple:tomap() +-- Should be NULL +tuple:tomap().fourth +-- Should be nil +type(tuple:tomap().fourth) +s:drop() + engine = nil test_run = nil ^ permalink raw reply [flat|nested] 4+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields 2018-08-22 15:31 ` Imeev Mergen @ 2018-08-24 10:23 ` Vladislav Shpilevoy 0 siblings, 0 replies; 4+ messages in thread From: Vladislav Shpilevoy @ 2018-08-24 10:23 UTC (permalink / raw) To: Imeev Mergen, tarantool-patches Hi! Thanks for the fixes! > commit 066db38393316c486b9edef0d71e854c3e4fdc78 > Author: Mergen Imeev <imeevma@gmail.com> > Date: Wed Aug 22 13:33:22 2018 +0300 > > lua: wrong 'tomap' work with nullable fields > > Tuple method 'tomap' in some cases worked improperly if tuple > length less than it should be according to space format. Fixed > in this patch. > > Closes #3631 > > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c > index 7ca4299..cd96152 100644 > --- a/src/box/lua/tuple.c > +++ b/src/box/lua/tuple.c > @@ -284,12 +284,11 @@ lbox_tuple_to_map(struct lua_State *L) > luaL_error(L, "Usage: tuple:tomap()"); > 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); > int field_count = (int)mp_decode_array(&pos); > int n_named = format->dict->name_count; > lua_createtable(L, field_count, n_named); > - for (int i = 0; i < n_named; ++i, ++field) { > + for (int i = 0; i < MIN(field_count, n_named); ++i) { It is not ok to calculate MIN on each iteration. Either N_named and field_count can not change during iteration. I fixed it in a separate commit on the branch. Please, look and squash. And send the patch as v2 to Vova D. > /* Access by name. */ > const char *name = format->dict->names[i]; > lua_pushstring(L, name); My diff (so far it is required by SOP since recently): diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c index cd961526c..5acbecb23 100644 --- a/src/box/lua/tuple.c +++ b/src/box/lua/tuple.c @@ -288,7 +288,8 @@ lbox_tuple_to_map(struct lua_State *L) int field_count = (int)mp_decode_array(&pos); int n_named = format->dict->name_count; lua_createtable(L, field_count, n_named); - for (int i = 0; i < MIN(field_count, n_named); ++i) { + int named_and_presented = MIN(field_count, n_named); + for (int i = 0; i < named_and_presented; ++i) { /* Access by name. */ const char *name = format->dict->names[i]; lua_pushstring(L, name); ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-08-24 10:23 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-22 10:43 [tarantool-patches] [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields imeevma 2018-08-22 14:16 ` [tarantool-patches] " Vladislav Shpilevoy 2018-08-22 15:31 ` Imeev Mergen 2018-08-24 10:23 ` Vladislav Shpilevoy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox