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 BFB4827445 for ; Fri, 13 Jul 2018 12:32:14 -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 LFhCu6S7kQtU for ; Fri, 13 Jul 2018 12:32:14 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 774C4273F0 for ; Fri, 13 Jul 2018 12:32:14 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 5/5] Methods for ephemeral space and its index References: From: Vladislav Shpilevoy Message-ID: <8ce351f7-8af2-8bed-1cac-7949a4082219@tarantool.org> Date: Fri, 13 Jul 2018 19:32:11 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: tarantool-patches@freelists.org, imeevma@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.