[tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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)
> +---
> +...