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 CAB1C2998E for ; Wed, 22 Aug 2018 11:31:09 -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 Pzuyu7yrzU3h for ; Wed, 22 Aug 2018 11:31:09 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (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 2BAFE29989 for ; Wed, 22 Aug 2018 11:31:08 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields References: <7d7cd9396f57d25a6bd05a24bf8677e7bb0fdd28.1534934562.git.imeevma@gmail.com> <90fa7a3e-d737-ea85-d4e4-0a0b0ef376d0@tarantool.org> From: Imeev Mergen Message-ID: <10fa1a23-736c-9c45-3023-9c69ed7793a1@tarantool.org> Date: Wed, 22 Aug 2018 18:31:04 +0300 MIME-Version: 1.0 In-Reply-To: <90fa7a3e-d737-ea85-d4e4-0a0b0ef376d0@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: Vladislav Shpilevoy , tarantool-patches@freelists.org 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 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