[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple
- From: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>
- To: tarantool-patches@xxxxxxxxxxxxx
- Date: Mon, 2 Apr 2018 13:26:46 +0300
Comments accounted.
On 29.03.2018 13:14, v.shpilevoy@xxxxxxxxxxxxx wrote:
See below 10 comments.
diff --git a/src/box/lua/space.cc
<http://space.cc> b/src/box/lua/space.cc <http://space.cc>
index 29a9aca..a8cfb14 100644
--- a/src/box/lua/space.cc <http://space.cc>
+++ b/src/box/lua/space.cc <http://space.cc>
@@ -32,6 +32,7 @@
#include "box/lua/tuple.h"
#include "lua/utils.h"
#include "lua/trigger.h"
+#include "box/schema.h"
extern "C" {
#include <lua.h>
@@ -174,6 +175,16 @@ lbox_fillspace(struct lua_State *L, struct space
*space, int i)
lua_pushstring(L, space->def->engine_name);
lua_settable(L, i);
+/* space.schema_version */
+lua_pushstring(L, "schema_version");
+luaL_pushuint64(L, box_schema_version());
+lua_settable(L, i);
1. Lets name it '_schema_version' - it is internal field.
+
+/* space._space */
+lua_pushstring(L, "_space");
+lua_pushlightuserdata(L, space);
+lua_settable(L, i);
2. Please, find a better name for the pointer. For example, 'ptr' - then it
will be accessed as space.ptr, that looks slightly better than space._space.
+static int
+lbox_space_ptr_cached(struct lua_State *L)
+{
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 740b885..c2645f7 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1420,7 +1420,7 @@ function box.schema.space.bless(space)
builtin.space_run_triggers(s, yesno)
end
space_mt.frommap = box.internal.space.frommap
- space_mt._ptr = box.internal.space._ptr
+ space_mt._cptr = box.internal.space._cptr
space_mt.__index = space_mt
setmetatable(space, space_mt)
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 42eaf79..f8583a2 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -176,12 +176,12 @@ lbox_fillspace(struct lua_State *L, struct space
*space, int i)
lua_settable(L, i);
/* space.schema_version */
- lua_pushstring(L, "schema_version");
+ lua_pushstring(L, "_schema_version");
luaL_pushuint64(L, box_schema_version());
lua_settable(L, i);
- /* space._space */
- lua_pushstring(L, "_space");
+ /* space._cptr */
+ lua_pushstring(L, "_cptr");
lua_pushlightuserdata(L, space);
lua_settable(L, i);
3. Add a comment, what this function does, why, and what arguments on a
stack
it expects, what and home many values returns.
+// take care about stack to call this function in frommap directly
+lua_getfield(L, 1, "schema_version");
4. A comment seems to be not linked with the code below. And please, do not
use '//' comments. In our code we use '/* ... */' for comments inside a
function. Note, that comment max length is 66 symbols, including indentation
before a comment.
+
+void *space = nullptr;
+if (schema_version == global_shema_version) {
+
5. Do not use nullptr. Everywhere in Tarantool code only NULL is used.
+bool tuple = false;
6. Lets return tuple by default, and name the option 'table' instead of
tuple. If a user wants a table, then he must pass the option {table = true}.
+if (argc == 3) {
+if (!lua_istable(L, 3))
+goto error;
+lua_getfield(L, 3, "tuple");
+if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
+goto error;
+tuple = lua_toboolean(L, -1);
+}
+
7. Double tabs prior to 'goto error'.
+if (tuple_fieldno_by_name(dict, key, key_len, key_hash, &fieldno))
+luaL_error(L, "Unknown field '%s'", key);
8. According to our Lua error returning convention, you can raise an
error either
on incorrect usage (bad argument type, for example), or on OOM(Out Of
Memory)
errors. On other errors you must return two values: nil and error object or
description. In this example you must return nil and "Unknown field
'%s'" string.
+lua_replace(L, 1);
+lua_settop(L, 1);
+return lbox_tuple_new(L);
@@ -397,56 +397,66 @@ static struct trigger on_alter_space_in_lua = {
RLIST_LINK_INITIALIZER, box_lua_space_new_or_delete, NULL, NULL
};
+/**
+ * Get a C pointer for space structure.
+ * Caches the result. Cleans the user(negative) stack itself.
+ * @param Lua space object at top of the Lua stack
+ * @return C structure pointer
+ */
static int
lbox_space_ptr_cached(struct lua_State *L)
{
if (lua_gettop(L) < 1 || !lua_istable(L, 1))
- luaL_error(L, "Usage: space:_ptr_cached()");
+ luaL_error(L, "Usage: space:_cptr()");
- // take care about stack to call this function in frommap directly
- lua_getfield(L, 1, "schema_version");
+ lua_getfield(L, 1, "_schema_version");
uint64_t schema_version = luaL_touint64(L, -1);
lua_pop(L, 1);
uint64_t global_shema_version = box_schema_version();
- void *space = nullptr;
+ void *space = NULL;
if (schema_version == global_shema_version) {
- lua_getfield(L, 1, "_space");
+ lua_getfield(L, 1, "_cptr");
space = (void *)lua_topointer(L, -1);
- lua_pop(L, 1);
} else {
lua_getfield(L, 1, "id");
uint32_t id = luaL_touint64(L, -1);
- lua_pop(L, 1);
space = space_by_id(id);
+ /** update the cache */
luaL_pushuint64(L, global_shema_version);
- lua_setfield(L, 1, "schema_version");
-
+ lua_setfield(L, 1, "_schema_version");
lua_pushlightuserdata(L, space);
- lua_setfield(L, 1, "_space");
+ lua_setfield(L, 1, "_cptr");
}
-
+ lua_pop(L, 1);
lua_pushlightuserdata(L, space);
return 1;
}
+/**
+ * Make a tuple or a table Lua object by map
+ * @param Lua space object
+ * @param Lua map table object
+ * @param Lua opts table object (optional)
+ * @return C structure pointer
+ */
static int
lbox_space_frommap(struct lua_State *L)
{
- struct tuple_dictionary *dict = nullptr;
- struct space *space = nullptr;
+ struct tuple_dictionary *dict = NULL;
+ struct space *space = NULL;
int argc = lua_gettop(L);
- bool tuple = false;
+ bool table = false;
if (argc < 2 || argc > 3 || !lua_istable(L, 2))
goto error;
if (argc == 3) {
if (!lua_istable(L, 3))
- goto error;
- lua_getfield(L, 3, "tuple");
+ goto error;
+ lua_getfield(L, 3, "table");
if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
- goto error;
- tuple = lua_toboolean(L, -1);
+ goto error;
+ table = lua_toboolean(L, -1);
}
lbox_space_ptr_cached(L);
@@ -461,11 +471,14 @@ lbox_space_frommap(struct lua_State *L)
size_t key_len;
const char *key = lua_tolstring(L, -2, &key_len);
uint32_t key_hash = lua_hashstring(L, -2);
- if (tuple_fieldno_by_name(dict, key, key_len, key_hash,
&fieldno))
- luaL_error(L, "Unknown field '%s'", key);
+ if (tuple_fieldno_by_name(dict, key, key_len, key_hash,
&fieldno)) {
+ lua_pushnil(L);
+ lua_pushstring(L, "Unknown field");
+ return 2;
+ }
lua_rawseti(L, -3, fieldno+1);
}
- if (!tuple)
+ if (table)
return 1;
lua_replace(L, 1);
@@ -557,7 +570,7 @@ box_lua_space_init(struct lua_State *L)
static const struct luaL_Reg space_internal_lib[] = {
{"frommap", lbox_space_frommap},
- {"_ptr", lbox_space_ptr_cached},
+ {"_cptr", lbox_space_ptr_cached},
{NULL, NULL}
};
luaL_register(L, "box.internal.space", space_internal_lib);
9. How about a comment what is happening here?
10. Add a test on nil and box.NULL fields. And test, that a result of
frommap() actually can be inserted or replaced into a space.
diff --git a/test/box/space_frommap.result b/test/box/space_frommap.result
index 884d78c..7a4b3db 100644
--- a/test/box/space_frommap.result
+++ b/test/box/space_frommap.result
@@ -29,21 +29,16 @@ subscriber = box.schema.space.create('subscriber',
{format = format})
...
subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
---
-- - 2
- - 4
- - 3
- - 1
+- [2, 4, 3, 1]
...
subscriber:frommap({ddd = 1, aaa = 2, bbb = 3})
---
-- - 2
- - 3
- - null
- - 1
+- [2, 3, null, 1]
...
subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
---
-- error: Unknown field 'eee'
+- null
+- Unknown field
...
subscriber:frommap()
---
@@ -53,23 +48,34 @@ subscriber:frommap({})
---
- []
...
-subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {tuple = true})
----
-- [2, 4, 3, 1]
-...
-subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {tuple = false})
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
---
- - 2
- 4
- 3
- 1
...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
+---
+- [2, 4, 3, 1]
+...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
+---
+- [2, null, 3, 1]
+...
subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
---
-- - 2
- - 4
- - 3
- - 1
+- [2, 4, 3, 1]
+...
+_ = subscriber:create_index('primary', {parts={'aaa'}})
+---
+...
+tuple = subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+...
+subscriber:replace(tuple)
+---
+- [2, 4, 3, 1]
...
subscriber:drop()
---
diff --git a/test/box/space_frommap.test.lua
b/test/box/space_frommap.test.lua
index 4760b3a..ff7ef98 100644
--- a/test/box/space_frommap.test.lua
+++ b/test/box/space_frommap.test.lua
@@ -14,7 +14,11 @@ subscriber:frommap({ddd = 1, aaa = 2, bbb = 3})
subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
subscriber:frommap()
subscriber:frommap({})
-subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {tuple = true})
-subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {tuple = false})
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
+_ = subscriber:create_index('primary', {parts={'aaa'}})
+tuple = subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+subscriber:replace(tuple)
subscriber:drop()
Other related posts:
