[tarantool-patches] Re: [PATCH 5/5] Methods for ephemeral space and its index

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

Thanks for the patch! See 10 comments below.

On 12/07/2018 14:16, imeevma@tarantool.org wrote:
> This patch defines most methods for index of
> ephemeral space and ephemeral space.
> 
> Closes #3375.
> ---
>   src/box/box.cc                    |   62 +
>   src/box/box.h                     |    9 +
>   src/box/index.cc                  |  172 +
>   src/box/index.h                   |  140 +
>   src/box/lua/info.h                |    4 -
>   src/box/lua/schema.lua            |  122 +
>   src/box/lua/space.cc              |  396 +-
>   test/box/ephemeral_space.result   | 7979 +++++++++++++++++++++++++++++++++++++
>   test/box/ephemeral_space.test.lua | 1694 ++++++++
>   9 files changed, 10573 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 795e3ee..e825735 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1171,6 +1171,68 @@ box_upsert(uint32_t space_id, uint32_t index_id, const char *tuple,
>   }
>   
>   int
> +box_ephemeral_select(struct space *space, uint32_t index_id,
> +		     int iterator, uint32_t offset, uint32_t limit,
> +		     const char *key, const char *key_end,
> +		     struct port *port)

1. This function is duplicate of box_select except transactions. Please,
remove this redundancy.

> diff --git a/src/box/index.cc b/src/box/index.cc
> index 188995e..d2d209f 100644
> --- a/src/box/index.cc
> +++ b/src/box/index.cc
> @@ -354,6 +354,131 @@ box_index_count(uint32_t space_id, uint32_t index_id, int type,
> +
> +int
> +box_ephemeral_index_get(struct space *space, uint32_t index_id, const char *key,
> +			const char *key_end, box_tuple_t **result)

2. Again duplication of a big function. Same about functions below.

> +{
> +	assert(key != NULL && key_end != NULL && result != NULL);
> +	mp_tuple_assert(key, key_end);
> +	struct index *index = index_find(space, index_id);
> +	if (index == NULL)
> +		return -1;
> +	if (!index->def->opts.is_unique) {
> +		diag_set(ClientError, ER_MORE_THAN_ONE_TUPLE);
> +		return -1;
> +	}
> +	uint32_t part_count = mp_decode_array(&key);
> +	if (exact_key_validate(index->def->key_def, key, part_count))
> +		return -1;
> +	if (index_get(index, key, part_count, result) != 0)
> +		return -1;
> +	rmean_collect(rmean_box, IPROTO_SELECT, 1);

3. I do not think we should track any ephemeral space things in statistics.
Same about rmean in other places.

> +	if (*result != NULL)
> +		tuple_bless(*result);
> +	return 0;
> +}
> +
> diff --git a/src/box/lua/info.h b/src/box/lua/info.h
> index 78cd9e6..bf4c613 100644
> --- a/src/box/lua/info.h
> +++ b/src/box/lua/info.h
> @@ -49,8 +49,4 @@ luaT_info_handler_create(struct info_handler *h, struct lua_State *L);
>   } /* extern "C" */
>   #endif /* defined(__cplusplus) */
>   
> -#if defined(__cplusplus)
> -} /* extern "C" */
> -#endif /* defined(__cplusplus) */

4. Please, rebase. This hunk is already in 2.0.

> -
>   #endif /* INCLUDES_TARANTOOL_LUA_INFO_H */
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 4374db9..81b9f3c 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -704,6 +708,370 @@ lbox_index_drop_ephemeral(struct lua_State *L)
> +static int
> +lbox_ephemeral_index_iterator(lua_State *L)
> +{
> +	if (lua_gettop(L) != 3 || !lua_istable(L, 1)) {
> +		return luaL_error(L, "Usage index:iterator(type, key)");
> +	}

5. Extra '{' and '}'.

> +	struct space *space = (struct space *)lua_checkephemeralspace(L, 1);
> +	int iterator = luaL_checknumber(L, 2);

6. It is allowed to use string iterator types: 'EQ', 'GE' etc.

> +	size_t mpkey_len;
> +	/* Key encoded by Lua */
> +	const char *mpkey = lua_tolstring(L, 3, &mpkey_len);
> +	struct iterator *it = box_ephemeral_index_iterator(space, 0, iterator,
> +							   mpkey,
> +							   mpkey + mpkey_len);
> +	if (it == NULL)
> +		return luaT_error(L);
> +
> +	assert(CTID_STRUCT_ITERATOR_REF != 0);
> +	struct iterator **ptr = (struct iterator **) luaL_pushcdata(L,
> +		CTID_STRUCT_ITERATOR_REF);

7. Where do you set GC for the iterator?

> +	*ptr = it; /* NULL handled by Lua, gc also set by Lua */
> +	return 1;
> +}
> +
> +/**
> + * Update tuple matched the provided key.
> + *
> + * @param Lua ephemeral space.
> + * @param Lua tuple - key.
> + * @param Lua tuple - operaions in case of update .
> + * @retval tuple or nil.
> + */
> +static int
> +lbox_ephemeral_index_update(lua_State *L)
> +{
> +	if (lua_gettop(L) != 3 || !lua_istable(L, 1) ||
> +	    (lua_type(L, 2) != LUA_TTABLE && luaT_istuple(L, 2) == NULL) ||
> +	    (lua_type(L, 3) != LUA_TTABLE && luaT_istuple(L, 3) == NULL))
> +		return luaL_error(L, "Usage index:update(key, ops)");
> +	struct space *space = (struct space *)lua_checkephemeralspace(L, 1);
> +	size_t key_len;
> +	const char *key = lbox_encode_tuple_on_gc(L, 2, &key_len);
> +	size_t ops_len;
> +	const char *ops = lbox_encode_tuple_on_gc(L, 3, &ops_len);
> +	struct tuple *result;
> +	if (box_ephemeral_update(space, 0, key, key + key_len,
> +				 ops, ops + ops_len, 1, &result) != 0)
> +		return luaT_error(L);
> +	return luaT_pushtupleornil(L, result);
> +}

8. Looks like most of this functions are almost complete
duplicates of ones from index.c. Please, remove duplication.
And move the functions into index.c.

> diff --git a/test/box/ephemeral_space.result b/test/box/ephemeral_space.result
> index d958ffe..0f80fa4 100644
> --- a/test/box/ephemeral_space.result
> +++ b/test/box/ephemeral_space.result
> @@ -1,3 +1,6 @@
> +test_run = require('test_run').new()
> +---
> +...
>   -- Ephemeral space: creation and dropping.
>   -- Simple creation.
>   s = box.schema.space.create_ephemeral()
> @@ -480,3 +483,7979 @@ s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
>   s:drop()
>   ---
>   ...
> +-- Ephemeral space: methods: insert
> +s = box.schema.space.create_ephemeral({field_count = 3})
> +---
> +...
> +i = s:create_index('a')

9. Please, reduce the test size. It is really huge and full of
redundancy:
* you do not need to create/drop space to test each statement;
* you do not need to fill each space with 10 records to test only one of them;
* it is not necessary to the same functionality with a set of almost
   identical tests like three insertions below.

> +---
> +...
> +s:insert{1}
> +---
> +- error: Tuple field count 1 does not match space field count 3
> +...
> +s:insert{2,2}
> +---
> +- error: Tuple field count 2 does not match space field count 3
> +...
> +s:insert{3,3,3}
> +---
> +- [3, 3, 3]
> +...
> +s:insert{4,4,4,4}
> +---
> +- error: Tuple field count 4 does not match space field count 3
> +...
> +s:drop()
> +---
> +...
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +function sort(t) table.sort(t, less) return t end

10. Unused function.

General recommendation: please, try to make .result file be at most 1000 lines.