[tarantool-patches] Re: [PATCH v2 1/1] schema:frommap() to create table or tuple

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Comments accounted.

On 29.03.2018 13:14, v.shpilevoy@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()