[tarantool-patches] Re: [PATCH 5/5] Methods for ephemeral space and its index
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jul 13 19:32:11 MSK 2018
Thanks for the patch! See 10 comments below.
On 12/07/2018 14:16, imeevma at 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.
More information about the Tarantool-patches
mailing list