[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple
Kirill Shcherbatov
kshcherbatov at tarantool.org
Mon Apr 2 13:26:46 MSK 2018
Comments accounted.
On 29.03.2018 13:14, v.shpilevoy at tarantool.org 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()
More information about the Tarantool-patches
mailing list