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

From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
To: tarantool-patches@xxxxxxxxxxxxx, imeevma@xxxxxxxxxxxxx
Date: Fri, 13 Jul 2018 19:32:11 +0300

Thanks for the patch! See 10 comments below.

On 12/07/2018 14:16, imeevma@xxxxxxxxxxxxx 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.

References:
- [tarantool-patches] [PATCH 0/5] Expose ephemeral spaces into Lua
  - From: imeevma
- [tarantool-patches] [PATCH 5/5] Methods for ephemeral space and its index
  - From: imeevma

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

Other related posts: