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

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

[Kirill Shcherbatov]

Closes #3282
---
 src/box/lua/schema.lua          | 14 +++++++
 src/box/lua/space.cc            | 86 +++++++++++++++++++++++++++++++++++++++++
 test/box/space_frommap.result   | 76 ++++++++++++++++++++++++++++++++++++
 test/box/space_frommap.test.lua | 20 ++++++++++
 4 files changed, 196 insertions(+)
 create mode 100644 test/box/space_frommap.result
 create mode 100644 test/box/space_frommap.test.lua

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 30c6bc6..afe81e8 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1419,6 +1419,20 @@ function box.schema.space.bless(space)
         end
         builtin.space_run_triggers(s, yesno)
     end
+    space_mt.frommap = function(space, map, options)
+        check_space_arg(space, 'frommap')
+        check_space_exists(space)
+        if map == nil then
+            -- allow C module process error itself
+            box.internal.space._table_frommap(space)
+        end
+        local ret = box.internal.space._table_frommap(space, map)
+        if options and options['tuple'] == true then
+            ret = box.tuple.new(ret)
+        end
+        return ret
+    end
+    space_mt._ptr = box.internal.space._ptr
     space_mt.__index = space_mt
 
     setmetatable(space, space_mt)
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 29a9aca..918f8f0 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/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);
+
+	/* space._space */
+	lua_pushstring(L, "_space");
+	lua_pushlightuserdata(L, space);
+	lua_settable(L, i);
+
 	lua_pushstring(L, "enabled");
 	lua_pushboolean(L, space_index(space, 0) != 0);
 	lua_settable(L, i);
@@ -386,6 +397,73 @@ static struct trigger on_alter_space_in_lua = {
 	RLIST_LINK_INITIALIZER, box_lua_space_new_or_delete, NULL, NULL
 };
 
+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()");
+
+	// take care about stack to call this function in frommap directly
+	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;
+	if (schema_version == global_shema_version) {
+		lua_getfield(L, 1, "_space");
+		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);
+
+		luaL_pushuint64(L, global_shema_version);
+		lua_setfield(L, 1, "schema_version");
+
+		lua_pushlightuserdata(L, space);
+		lua_setfield(L, 1, "_space");
+	}
+
+	lua_pushlightuserdata(L, space);
+	return 1;
+}
+
+static int
+lbox_space_table_frommap(struct lua_State *L)
+{
+	int res = 0;
+	struct tuple_dictionary *dict = nullptr;
+	struct space *space = nullptr;
+	int argc = lua_gettop(L);
+	if (argc != 2)
+		goto error;
+
+	res = lbox_space_ptr_cached(L);
+	space = (struct space *)lua_topointer(L, -1);
+	lua_pop(L, res);
+
+	dict = space->format->dict;
+	lua_createtable(L, space->def->field_count, 0);
+
+	lua_pushnil(L);
+	while (lua_next(L, 2) != 0) {
+		uint32_t fieldno;
+		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);
+		lua_rawseti(L, -3, fieldno+1);
+	}
+	return 1;
+error:
+	luaL_error(L, "Usage: space:frommap(map, opts)");
+	return 1;
+}
+
 void
 box_lua_space_init(struct lua_State *L)
 {
@@ -464,4 +542,12 @@ box_lua_space_init(struct lua_State *L)
 	lua_pushnumber(L, VCLOCK_MAX);
 	lua_setfield(L, -2, "REPLICA_MAX");
 	lua_pop(L, 2); /* box, schema */
+
+	static const struct luaL_Reg space_internal_lib[] = {
+		{"_table_frommap", lbox_space_table_frommap},
+		{"_ptr", lbox_space_ptr_cached},
+		{NULL, NULL}
+	};
+	luaL_register(L, "box.internal.space", space_internal_lib);
+	lua_pop(L, 1);
 }
diff --git a/test/box/space_frommap.result b/test/box/space_frommap.result
new file mode 100644
index 0000000..811aa60
--- /dev/null
+++ b/test/box/space_frommap.result
@@ -0,0 +1,76 @@
+-- box.tuple test
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+---
+- true
+...
+format = {}
+---
+...
+format[1] = {name = 'aaa', type = 'unsigned'}
+---
+...
+format[2] = {name = 'bbb', type = 'unsigned'}
+---
+...
+format[3] = {name = 'ccc', type = 'unsigned'}
+---
+...
+format[4] = {name = 'ddd', type = 'unsigned'}
+---
+...
+subscriber = box.schema.space.create('subscriber', {format = format})
+---
+...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+subscriber:frommap({ddd = 1, aaa = 2, bbb = 3})
+---
+- - 2
+  - 3
+  - null
+  - 1
+...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
+---
+- error: 'builtin/box/schema.lua..."]:<line>: Unknown field ''eee'''
+...
+subscriber:frommap()
+---
+- error: 'builtin/box/schema.lua..."]:<line>: Usage: space:frommap(map, opts)'
+...
+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})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+subscriber:drop()
+---
+...
diff --git a/test/box/space_frommap.test.lua b/test/box/space_frommap.test.lua
new file mode 100644
index 0000000..4760b3a
--- /dev/null
+++ b/test/box/space_frommap.test.lua
@@ -0,0 +1,20 @@
+-- box.tuple test
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+
+format = {}
+format[1] = {name = 'aaa', type = 'unsigned'}
+format[2] = {name = 'bbb', type = 'unsigned'}
+format[3] = {name = 'ccc', type = 'unsigned'}
+format[4] = {name = 'ddd', type = 'unsigned'}
+subscriber = box.schema.space.create('subscriber', {format = format})
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+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}, {dummy = true})
+subscriber:drop()
-- 
2.7.4

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

[Kirill Shcherbatov]

Closes #3282
---
 src/box/lua/schema.lua        |   2 +
 src/box/lua/space.cc          | 100 ++++++++++++++++++++++++++++++++++++++++++
 src/box/lua/tuple.c           |   2 +-
 src/box/lua/tuple.h           |   3 ++
 test/box/space_frommap.result |  76 ++++++++++++++++++++++++++++++++
 5 files changed, 182 insertions(+), 1 deletion(-)
 create mode 100644 test/box/space_frommap.result

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 30c6bc6..740b885 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1419,6 +1419,8 @@ function box.schema.space.bless(space)
         end
         builtin.space_run_triggers(s, yesno)
     end
+    space_mt.frommap = box.internal.space.frommap
+    space_mt._ptr = box.internal.space._ptr
     space_mt.__index = space_mt
 
     setmetatable(space, space_mt)
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 29a9aca..a8cfb14 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/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);
+
+	/* space._space */
+	lua_pushstring(L, "_space");
+	lua_pushlightuserdata(L, space);
+	lua_settable(L, i);
+
 	lua_pushstring(L, "enabled");
 	lua_pushboolean(L, space_index(space, 0) != 0);
 	lua_settable(L, i);
@@ -386,6 +397,87 @@ static struct trigger on_alter_space_in_lua = {
 	RLIST_LINK_INITIALIZER, box_lua_space_new_or_delete, NULL, NULL
 };
 
+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()");
+
+	// take care about stack to call this function in frommap directly
+	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;
+	if (schema_version == global_shema_version) {
+		lua_getfield(L, 1, "_space");
+		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);
+
+		luaL_pushuint64(L, global_shema_version);
+		lua_setfield(L, 1, "schema_version");
+
+		lua_pushlightuserdata(L, space);
+		lua_setfield(L, 1, "_space");
+	}
+
+	lua_pushlightuserdata(L, space);
+	return 1;
+}
+
+static int
+lbox_space_frommap(struct lua_State *L)
+{
+	int res = 0;
+	struct tuple_dictionary *dict = nullptr;
+	struct space *space = nullptr;
+	int argc = lua_gettop(L);
+	bool tuple = 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");
+		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
+				goto error;
+		tuple = lua_toboolean(L, -1);
+	}
+
+	res = lbox_space_ptr_cached(L);
+	space = (struct space *)lua_topointer(L, -1);
+	lua_pop(L, res);
+
+	dict = space->format->dict;
+	lua_createtable(L, space->def->field_count, 0);
+
+	lua_pushnil(L);
+	while (lua_next(L, 2) != 0) {
+		uint32_t fieldno;
+		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);
+		lua_rawseti(L, -3, fieldno+1);
+	}
+	if (!tuple)
+		return 1;
+
+	lua_replace(L, 1);
+	lua_settop(L, 1);
+	return lbox_tuple_new(L);
+error:
+	luaL_error(L, "Usage: space:frommap(map, opts)");
+	return 1;
+}
+
 void
 box_lua_space_init(struct lua_State *L)
 {
@@ -464,4 +556,12 @@ box_lua_space_init(struct lua_State *L)
 	lua_pushnumber(L, VCLOCK_MAX);
 	lua_setfield(L, -2, "REPLICA_MAX");
 	lua_pop(L, 2); /* box, schema */
+
+	static const struct luaL_Reg space_internal_lib[] = {
+		{"frommap", lbox_space_frommap},
+		{"_ptr", lbox_space_ptr_cached},
+		{NULL, NULL}
+	};
+	luaL_register(L, "box.internal.space", space_internal_lib);
+	lua_pop(L, 1);
 }
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 7ca4299..92a7185 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-static int
+int
 lbox_tuple_new(lua_State *L)
 {
 	int argc = lua_gettop(L);
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 0965644..5d7062e 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
 
 /** \endcond public */
 
+int
+lbox_tuple_new(struct lua_State *L);
+
 static inline int
 luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
 {
diff --git a/test/box/space_frommap.result b/test/box/space_frommap.result
new file mode 100644
index 0000000..884d78c
--- /dev/null
+++ b/test/box/space_frommap.result
@@ -0,0 +1,76 @@
+-- box.tuple test
+env = require('test_run')
+---
+...
+test_run = env.new()
+---
+...
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+---
+- true
+...
+format = {}
+---
+...
+format[1] = {name = 'aaa', type = 'unsigned'}
+---
+...
+format[2] = {name = 'bbb', type = 'unsigned'}
+---
+...
+format[3] = {name = 'ccc', type = 'unsigned'}
+---
+...
+format[4] = {name = 'ddd', type = 'unsigned'}
+---
+...
+subscriber = box.schema.space.create('subscriber', {format = format})
+---
+...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+subscriber:frommap({ddd = 1, aaa = 2, bbb = 3})
+---
+- - 2
+  - 3
+  - null
+  - 1
+...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
+---
+- error: Unknown field 'eee'
+...
+subscriber:frommap()
+---
+- error: 'Usage: space:frommap(map, opts)'
+...
+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})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+subscriber:drop()
+---
+...
-- 
2.7.4

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

[Kirill Shcherbatov]

1. Deleted redundant lua_pop in  lbox_space_frommap
2. Included forgotten new frommap test case

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index a8cfb14..42eaf79 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -434,7 +434,6 @@ lbox_space_ptr_cached(struct lua_State *L)
  static int
  lbox_space_frommap(struct lua_State *L)
  {
-	int res = 0;
  	struct tuple_dictionary *dict = nullptr;
  	struct space *space = nullptr;
  	int argc = lua_gettop(L);
@@ -450,9 +449,8 @@ lbox_space_frommap(struct lua_State *L)
  		tuple = lua_toboolean(L, -1);
  	}

-	res = lbox_space_ptr_cached(L);
+	lbox_space_ptr_cached(L);
  	space = (struct space *)lua_topointer(L, -1);
-	lua_pop(L, res);

  	dict = space->format->dict;
  	lua_createtable(L, space->def->field_count, 0);
diff --git a/test/box/space_frommap.test.lua 
b/test/box/space_frommap.test.lua
new file mode 100644
index 0000000..4760b3a
--- /dev/null
+++ b/test/box/space_frommap.test.lua
@@ -0,0 +1,20 @@
+-- box.tuple test
+env = require('test_run')
+test_run = env.new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to 
'.lua...\"]:<line>: '")
+
+format = {}
+format[1] = {name = 'aaa', type = 'unsigned'}
+format[2] = {name = 'bbb', type = 'unsigned'}
+format[3] = {name = 'ccc', type = 'unsigned'}
+format[4] = {name = 'ddd', type = 'unsigned'}
+subscriber = box.schema.space.create('subscriber', {format = format})
+subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+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}, {dummy = true})
+subscriber:drop()


On 28.03.2018 21:28, Kirill Shcherbatov wrote:
> Closes #3282
> ---
>   src/box/lua/schema.lua        |   2 +
>   src/box/lua/space.cc          | 100 ++++++++++++++++++++++++++++++++++++++++++
>   src/box/lua/tuple.c           |   2 +-
>   src/box/lua/tuple.h           |   3 ++
>   test/box/space_frommap.result |  76 ++++++++++++++++++++++++++++++++
>   5 files changed, 182 insertions(+), 1 deletion(-)
>   create mode 100644 test/box/space_frommap.result
> 
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 30c6bc6..740b885 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1419,6 +1419,8 @@ function box.schema.space.bless(space)
>           end
>           builtin.space_run_triggers(s, yesno)
>       end
> +    space_mt.frommap = box.internal.space.frommap
> +    space_mt._ptr = box.internal.space._ptr
>       space_mt.__index = space_mt
>   
>       setmetatable(space, space_mt)
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 29a9aca..a8cfb14 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/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);
> +
> +	/* space._space */
> +	lua_pushstring(L, "_space");
> +	lua_pushlightuserdata(L, space);
> +	lua_settable(L, i);
> +
>   	lua_pushstring(L, "enabled");
>   	lua_pushboolean(L, space_index(space, 0) != 0);
>   	lua_settable(L, i);
> @@ -386,6 +397,87 @@ static struct trigger on_alter_space_in_lua = {
>   	RLIST_LINK_INITIALIZER, box_lua_space_new_or_delete, NULL, NULL
>   };
>   
> +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()");
> +
> +	// take care about stack to call this function in frommap directly
> +	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;
> +	if (schema_version == global_shema_version) {
> +		lua_getfield(L, 1, "_space");
> +		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);
> +
> +		luaL_pushuint64(L, global_shema_version);
> +		lua_setfield(L, 1, "schema_version");
> +
> +		lua_pushlightuserdata(L, space);
> +		lua_setfield(L, 1, "_space");
> +	}
> +
> +	lua_pushlightuserdata(L, space);
> +	return 1;
> +}
> +
> +static int
> +lbox_space_frommap(struct lua_State *L)
> +{
> +	int res = 0;
> +	struct tuple_dictionary *dict = nullptr;
> +	struct space *space = nullptr;
> +	int argc = lua_gettop(L);
> +	bool tuple = 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");
> +		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
> +				goto error;
> +		tuple = lua_toboolean(L, -1);
> +	}
> +
> +	res = lbox_space_ptr_cached(L);
> +	space = (struct space *)lua_topointer(L, -1);
> +	lua_pop(L, res);
> +
> +	dict = space->format->dict;
> +	lua_createtable(L, space->def->field_count, 0);
> +
> +	lua_pushnil(L);
> +	while (lua_next(L, 2) != 0) {
> +		uint32_t fieldno;
> +		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);
> +		lua_rawseti(L, -3, fieldno+1);
> +	}
> +	if (!tuple)
> +		return 1;
> +
> +	lua_replace(L, 1);
> +	lua_settop(L, 1);
> +	return lbox_tuple_new(L);
> +error:
> +	luaL_error(L, "Usage: space:frommap(map, opts)");
> +	return 1;
> +}
> +
>   void
>   box_lua_space_init(struct lua_State *L)
>   {
> @@ -464,4 +556,12 @@ box_lua_space_init(struct lua_State *L)
>   	lua_pushnumber(L, VCLOCK_MAX);
>   	lua_setfield(L, -2, "REPLICA_MAX");
>   	lua_pop(L, 2); /* box, schema */
> +
> +	static const struct luaL_Reg space_internal_lib[] = {
> +		{"frommap", lbox_space_frommap},
> +		{"_ptr", lbox_space_ptr_cached},
> +		{NULL, NULL}
> +	};
> +	luaL_register(L, "box.internal.space", space_internal_lib);
> +	lua_pop(L, 1);
>   }
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 7ca4299..92a7185 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
>   	return *(struct tuple **) data;
>   }
>   
> -static int
> +int
>   lbox_tuple_new(lua_State *L)
>   {
>   	int argc = lua_gettop(L);
> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index 0965644..5d7062e 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
>   
>   /** \endcond public */
>   
> +int
> +lbox_tuple_new(struct lua_State *L);
> +
>   static inline int
>   luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
>   {
> diff --git a/test/box/space_frommap.result b/test/box/space_frommap.result
> new file mode 100644
> index 0000000..884d78c
> --- /dev/null
> +++ b/test/box/space_frommap.result
> @@ -0,0 +1,76 @@
> +-- box.tuple test
> +env = require('test_run')
> +---
> +...
> +test_run = env.new()
> +---
> +...
> +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
> +---
> +- true
> +...
> +format = {}
> +---
> +...
> +format[1] = {name = 'aaa', type = 'unsigned'}
> +---
> +...
> +format[2] = {name = 'bbb', type = 'unsigned'}
> +---
> +...
> +format[3] = {name = 'ccc', type = 'unsigned'}
> +---
> +...
> +format[4] = {name = 'ddd', type = 'unsigned'}
> +---
> +...
> +subscriber = box.schema.space.create('subscriber', {format = format})
> +---
> +...
> +subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +- - 2
> +  - 4
> +  - 3
> +  - 1
> +...
> +subscriber:frommap({ddd = 1, aaa = 2, bbb = 3})
> +---
> +- - 2
> +  - 3
> +  - null
> +  - 1
> +...
> +subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
> +---
> +- error: Unknown field 'eee'
> +...
> +subscriber:frommap()
> +---
> +- error: 'Usage: space:frommap(map, opts)'
> +...
> +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})
> +---
> +- - 2
> +  - 4
> +  - 3
> +  - 1
> +...
> +subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
> +---
> +- - 2
> +  - 4
> +  - 3
> +  - 1
> +...
> +subscriber:drop()
> +---
> +...
> 

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

[v.shpilevoy]

[-- Attachment #1: Type: text/plain, Size: 2986 bytes --]

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)
> +{

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);

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.



[-- Attachment #2: Type: text/html, Size: 15052 bytes --]

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

[Kirill Shcherbatov]

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()

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

[v.shpilevoy]

Hello. Thanks for your contributing! A several (5) minor fixes are left:

1. Please, describe the patch in a commit message body. It would be good, if you
write why the feature is needed, who needs this, and very short description of
the feature itself: API usage example, options description. See Vladimir Davydov
patches as the perfect example.

> 	/* space.schema_version */
> 	lua_pushstring(L, "_schema_version");
> 	luaL_pushuint64(L, box_schema_version());
> 	lua_settable(L, i);

2. Please, update the comment too: /* space._schema_version */ (added '_').

> /**
>  * 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)

3. Please, use @retval instead of @return, and put '.' at the end of a sentence.

4. Seems like the following test crashes Tarantool. Please, fix it.

box.cfg{}
format = {{'field1', 'unsigned'}}
s = box.schema.create_space('test', {format = format})
pk = s:create_index('pk')
s:drop()
s:frommap({field1 = 100})

Process 67761 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xe8)
    frame #0: 0x0000000100100b68 tarantool`lbox_space_frommap(L=0x0000000103200378) at space.cc:465
   462 		lbox_space_ptr_cached(L);
   463 		space = (struct space *)lua_topointer(L, -1);
   464 	
-> 465 		dict = space->format->dict;
   466 		lua_createtable(L, space->def->field_count, 0);
   467 	
   468 		lua_pushnil(L);
Target 0: (tarantool) stopped.



> 		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);


5. Please, return field name into error message. In the previous patch the error message
was ok. Just do not throw it. And put spaces before and after arithmetic operations.

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

[Kirill Shcherbatov]

I've also updated commit message content.

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index f8583a2..2ff9a11 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -38,6 +38,7 @@ extern "C" {
  	#include <lua.h>
  	#include <lauxlib.h>
  	#include <lualib.h>
+	#include <trivia/util.h>
  } /* extern "C" */

  #include "box/space.h"
@@ -175,7 +176,7 @@ 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 */
+	/* space._schema_version */
  	lua_pushstring(L, "_schema_version");
  	luaL_pushuint64(L, box_schema_version());
  	lua_settable(L, i);
@@ -400,8 +401,8 @@ static struct trigger on_alter_space_in_lua = {
  /**
   * 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
+ * @param Lua space object at top of the Lua stack.
+ * @retval C structure pointer.
   */
  static int
  lbox_space_ptr_cached(struct lua_State *L)
@@ -422,6 +423,8 @@ lbox_space_ptr_cached(struct lua_State *L)
  		lua_getfield(L, 1, "id");
  		uint32_t id = luaL_touint64(L, -1);
  		space = space_by_id(id);
+		if (!space)
+			luaL_error(L, "Specified space is not exists");

  		/** update the cache */
  		luaL_pushuint64(L, global_shema_version);
@@ -435,11 +438,11 @@ lbox_space_ptr_cached(struct lua_State *L)
  }

  /**
- * 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
+ * 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).
+ * @retval C structure pointer.
   */
  static int
  lbox_space_frommap(struct lua_State *L)
@@ -461,7 +464,6 @@ lbox_space_frommap(struct lua_State *L)

  	lbox_space_ptr_cached(L);
  	space = (struct space *)lua_topointer(L, -1);
-
  	dict = space->format->dict;
  	lua_createtable(L, space->def->field_count, 0);

@@ -472,8 +474,9 @@ lbox_space_frommap(struct lua_State *L)
  		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)) {
+			const char *err_msg = tt_sprintf("Unknown field '%s'", key);
  			lua_pushnil(L);
-			lua_pushstring(L, "Unknown field");
+			lua_pushstring(L, err_msg);
  			return 2;
  		}
  		lua_rawseti(L, -3, fieldno+1);


On 02.04.2018 14:19, v.shpilevoy@tarantool.org wrote:
> Hello. Thanks for your contributing! A several (5) minor fixes are left:
> 
> 1. Please, describe the patch in a commit message body. It would be good, if you
> write why the feature is needed, who needs this, and very short description of
> the feature itself: API usage example, options description. See Vladimir Davydov
> patches as the perfect example.
> 
>> 	/* space.schema_version */
>> 	lua_pushstring(L, "_schema_version");
>> 	luaL_pushuint64(L, box_schema_version());
>> 	lua_settable(L, i);
> 
> 2. Please, update the comment too: /* space._schema_version */ (added '_').
> 
>> /**
>>   * 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)
> 
> 3. Please, use @retval instead of @return, and put '.' at the end of a sentence.
> 
> 4. Seems like the following test crashes Tarantool. Please, fix it.
> 
> box.cfg{}
> format = {{'field1', 'unsigned'}}
> s = box.schema.create_space('test', {format = format})
> pk = s:create_index('pk')
> s:drop()
> s:frommap({field1 = 100})
> 
> Process 67761 stopped
> * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xe8)
>      frame #0: 0x0000000100100b68 tarantool`lbox_space_frommap(L=0x0000000103200378) at space.cc:465
>     462 		lbox_space_ptr_cached(L);
>     463 		space = (struct space *)lua_topointer(L, -1);
>     464 	
> -> 465 		dict = space->format->dict;
>     466 		lua_createtable(L, space->def->field_count, 0);
>     467 	
>     468 		lua_pushnil(L);
> Target 0: (tarantool) stopped.
> 
> 
> 
>> 		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);
> 
> 
> 5. Please, return field name into error message. In the previous patch the error message
> was ok. Just do not throw it. And put spaces before and after arithmetic operations.
> 
> 

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

[Vladislav Shpilevoy]

Hello. Please, consider 4 comments below.
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 29a9aca..2ff9a11 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -32,11 +32,13 @@
>   #include "box/lua/tuple.h"
>   #include "lua/utils.h"
>   #include "lua/trigger.h"
> +#include "box/schema.h"
>   
>   extern "C" {
>   	#include <lua.h>
>   	#include <lauxlib.h>
>   	#include <lualib.h>
> +	#include <trivia/util.h>
>   } /* extern "C" */
1. Schema.h is included below. Trivia/util.h is not needed here.

> +		space = space_by_id(id);
> +		if (!space)
> +			luaL_error(L, "Specified space is not exists");
> +
2. Please, for pointers use explicit != NULL.

3. As I noted earlier, a function must not raise an error. It must 
return nil + error message/object.
And please, check your comments grammar: you can not say "is not 
exists". Perhaps,
you meant "does not exist".

4. I do not see my crash test in your space_frommap.test.lua. When I 
provide you a test, or you
found an own, you must add it to the patch.

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

[Kirill Shcherbatov]

On 03.04.2018 12:47, Vladislav Shpilevoy wrote:
> Hello. Please, consider 4 comments below.
>> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
>> index 29a9aca..2ff9a11 100644
>> --- a/src/box/lua/space.cc
>> +++ b/src/box/lua/space.cc
>> @@ -32,11 +32,13 @@
>>   #include "box/lua/tuple.h"
>>   #include "lua/utils.h"
>>   #include "lua/trigger.h"
>> +#include "box/schema.h"
>>   extern "C" {
>>       #include <lua.h>
>>       #include <lauxlib.h>
>>       #include <lualib.h>
>> +    #include <trivia/util.h>
>>   } /* extern "C" */
> 1. Schema.h is included below. Trivia/util.h is not needed here.
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 2ff9a11..9004c0e 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -38,7 +38,6 @@ extern "C" {
  	#include <lua.h>
  	#include <lauxlib.h>
  	#include <lualib.h>
-	#include <trivia/util.h>
  } /* extern "C" */

  #include "box/space.h"
> 
>> +        space = space_by_id(id);
>> +        if (!space)
>> +            luaL_error(L, "Specified space is not exists");
>> +
> 2. Please, for pointers use explicit != NULL.
> 
> 3. As I noted earlier, a function must not raise an error. It must 
> return nil + error message/object.
> And please, check your comments grammar: you can not say "is not 
> exists". Perhaps,
> you meant "does not exist".
> 
@@ -423,8 +422,14 @@ lbox_space_ptr_cached(struct lua_State *L)
  		lua_getfield(L, 1, "id");
  		uint32_t id = luaL_touint64(L, -1);
  		space = space_by_id(id);
-		if (!space)
-			luaL_error(L, "Specified space is not exists");
+		if (space == NULL) {
+			const char *err_msg;
+			err_msg = tt_sprintf("Space with id '%d' doesn't exists",
+			                     id);
+			lua_pushnil(L);
+			lua_pushstring(L, err_msg);
+			return 2;
+		}

  		/** update the cache */
  		luaL_pushuint64(L, global_shema_version);
@@ -462,8 +467,14 @@ lbox_space_frommap(struct lua_State *L)
  		table = lua_toboolean(L, -1);
  	}

-	lbox_space_ptr_cached(L);
-	space = (struct space *)lua_topointer(L, -1);
+	if (lbox_space_ptr_cached(L) == 1) {
+		space = (struct space *) lua_topointer(L, -1);
+	} else {
+		const char *err_msg = lua_tostring(L, -1);
+		lua_pushnil(L);
+		lua_pushstring(L, err_msg);
+		return 2;
+	}
  	dict = space->format->dict;
  	lua_createtable(L, space->def->field_count, 0);
> 4. I do not see my crash test in your space_frommap.test.lua. When I 
> provide you a test, or you
> found an own, you must add it to the patch.
> 
There is the same test case here:

diff --git a/test/box/space_frommap.test.lua 
b/test/box/space_frommap.test.lua
index ff7ef98..647d761 100644
--- a/test/box/space_frommap.test.lua
+++ b/test/box/space_frommap.test.lua
@@ -21,4 +21,7 @@ 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)
+_ = box.internal.space._cptr(subscriber)
  subscriber:drop()
+_ = subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+_ = box.internal.space._cptr(subscriber)

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

[Kirill Shcherbatov]

This feature is designed to build tuple or table object by map.
Tuple output is default. If you wish to make a table, specify table = true option.
subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})

Closes #3282
---
 src/box/lua/schema.lua  |   1 +
 src/box/lua/space.cc    | 123 ++++++++++++++++++++++++++++++++++++++++++++++++
 src/box/lua/tuple.c     |   2 +-
 src/box/lua/tuple.h     |   3 ++
 test/box/tuple.result   |  95 +++++++++++++++++++++++++++++++++++++
 test/box/tuple.test.lua |  29 ++++++++++++
 6 files changed, 252 insertions(+), 1 deletion(-)

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 30c6bc6..323c2d6 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1419,6 +1419,7 @@ function box.schema.space.bless(space)
         end
         builtin.space_run_triggers(s, yesno)
     end
+    space_mt.frommap = box.internal.space.frommap
     space_mt.__index = space_mt
 
     setmetatable(space, space_mt)
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 29a9aca..2723baf 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -174,6 +174,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);
+
+	/* space._cptr */
+	lua_pushstring(L, "_cptr");
+	lua_pushlightuserdata(L, space);
+	lua_settable(L, i);
+
 	lua_pushstring(L, "enabled");
 	lua_pushboolean(L, space_index(space, 0) != 0);
 	lua_settable(L, i);
@@ -386,6 +396,111 @@ 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.
+ * @retval not nil C structure pointer.
+ * @retval nil, err Can not find underlying space, error
+ *         description is returned.
+ */
+static int
+lbox_space_ptr_cached(struct lua_State *L)
+{
+	if (lua_gettop(L) < 1 || !lua_istable(L, 1))
+		luaL_error(L, "Usage: space:_cptr()");
+
+	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 = NULL;
+	if (schema_version == global_shema_version) {
+		lua_getfield(L, 1, "_cptr");
+		space = (void *)lua_topointer(L, -1);
+	} else {
+		lua_getfield(L, 1, "id");
+		uint32_t id = luaL_touint64(L, -1);
+		space = space_by_id(id);
+		if (space == NULL) {
+			lua_pushnil(L);
+			lua_pushstring(L, tt_sprintf("Space with id '%d' "\
+						     "doesn't exist", id));
+			return 2;
+		}
+
+		/** update the cache */
+		luaL_pushuint64(L, global_shema_version);
+		lua_setfield(L, 1, "_schema_version");
+		lua_pushlightuserdata(L, 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).
+ * @retval not nil A tuple or a table conforming to a space
+ *         format.
+ * @retval nil, err Can not built a tuple. A reason is returned in
+ *         the second value.
+ */
+static int
+lbox_space_frommap(struct lua_State *L)
+{
+	struct tuple_dictionary *dict = NULL;
+	struct space *space = NULL;
+	int rc, argc = lua_gettop(L);
+	bool table = false;
+	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
+		goto usage_error;
+	if (argc == 3) {
+		if (!lua_istable(L, 3))
+			goto usage_error;
+		lua_getfield(L, 3, "table");
+		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
+			goto usage_error;
+		table = lua_toboolean(L, -1);
+	}
+
+	rc = lbox_space_ptr_cached(L);
+	if (rc != 1)
+		return rc;
+	space = (struct space *) lua_topointer(L, -1);
+	dict = space->format->dict;
+	lua_createtable(L, space->def->field_count, 0);
+
+	lua_pushnil(L);
+	while (lua_next(L, 2) != 0) {
+		uint32_t fieldno;
+		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)) {
+			lua_pushnil(L);
+			lua_pushstring(L, tt_sprintf("Unknown field '%s'",
+						     key));
+			return 2;
+		}
+		lua_rawseti(L, -3, fieldno+1);
+	}
+	if (table)
+		return 1;
+
+	lua_replace(L, 1);
+	lua_settop(L, 1);
+	return lbox_tuple_new(L);
+usage_error:
+	return luaL_error(L, "Usage: space:frommap(map, opts)");
+}
+
 void
 box_lua_space_init(struct lua_State *L)
 {
@@ -464,4 +579,12 @@ box_lua_space_init(struct lua_State *L)
 	lua_pushnumber(L, VCLOCK_MAX);
 	lua_setfield(L, -2, "REPLICA_MAX");
 	lua_pop(L, 2); /* box, schema */
+
+	static const struct luaL_Reg space_internal_lib[] = {
+		{"frommap", lbox_space_frommap},
+		{"_cptr", lbox_space_ptr_cached},
+		{NULL, NULL}
+	};
+	luaL_register(L, "box.internal.space", space_internal_lib);
+	lua_pop(L, 1);
 }
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 7ca4299..92a7185 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-static int
+int
 lbox_tuple_new(lua_State *L)
 {
 	int argc = lua_gettop(L);
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 0965644..5d7062e 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
 
 /** \endcond public */
 
+int
+lbox_tuple_new(struct lua_State *L);
+
 static inline int
 luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
 {
diff --git a/test/box/tuple.result b/test/box/tuple.result
index ec5ed90..2dd8f0d 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1060,6 +1060,101 @@ box.tuple.bsize == t.bsize
 ---
 - true
 ...
+--
+-- gh-3282: space:frommap().
+--
+format = {}
+---
+...
+format[1] = {name = 'aaa', type = 'unsigned'}
+---
+...
+format[2] = {name = 'bbb', type = 'unsigned'}
+---
+...
+format[3] = {name = 'ccc', type = 'unsigned'}
+---
+...
+format[4] = {name = 'ddd', type = 'unsigned'}
+---
+...
+s = box.schema.create_space('test', {format = format})
+---
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+- [2, 4, 3, 1]
+...
+s:frommap({ddd = 1, aaa = 2, bbb = 3})
+---
+- [2, 3, null, 1]
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
+---
+- null
+- Unknown field 'eee'
+...
+s:frommap()
+---
+- error: 'Usage: space:frommap(map, opts)'
+...
+s:frommap({})
+---
+- []
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
+---
+- [2, 4, 3, 1]
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
+---
+- [2, null, 3, 1]
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
+---
+- [2, 4, 3, 1]
+...
+_ = s:create_index('primary', {parts = {'aaa'}})
+---
+...
+tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+...
+s:replace(tuple)
+---
+- [2, 4, 3, 1]
+...
+_, err = box.internal.space._cptr(s)
+---
+...
+assert(not err)
+---
+- true
+...
+s:drop()
+---
+...
+_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+...
+assert(err ~= nil)
+---
+- true
+...
+_, err = box.internal.space._cptr(s)
+---
+...
+assert(err ~= nil)
+---
+- true
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index 0b3392d..03e6194 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -349,4 +349,33 @@ box.tuple.update == t.update
 box.tuple.upsert == t.upsert
 box.tuple.bsize == t.bsize
 
+--
+-- gh-3282: space:frommap().
+--
+format = {}
+format[1] = {name = 'aaa', type = 'unsigned'}
+format[2] = {name = 'bbb', type = 'unsigned'}
+format[3] = {name = 'ccc', type = 'unsigned'}
+format[4] = {name = 'ddd', type = 'unsigned'}
+s = box.schema.create_space('test', {format = format})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+s:frommap({ddd = 1, aaa = 2, bbb = 3})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
+s:frommap()
+s:frommap({})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
+_ = s:create_index('primary', {parts = {'aaa'}})
+tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+s:replace(tuple)
+_, err = box.internal.space._cptr(s)
+assert(not err)
+s:drop()
+_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+assert(err ~= nil)
+_, err = box.internal.space._cptr(s)
+assert(err ~= nil)
+
 test_run:cmd("clear filter")
-- 
2.7.4

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

[Vladislav Shpilevoy]

LGTM. Vova, please, take a look.

04.04.2018 19:35, Kirill Shcherbatov пишет:
> This feature is designed to build tuple or table object by map.
> Tuple output is default. If you wish to make a table, specify table = true option.
> subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
>
> Closes #3282
> ---
>   src/box/lua/schema.lua  |   1 +
>   src/box/lua/space.cc    | 123 ++++++++++++++++++++++++++++++++++++++++++++++++
>   src/box/lua/tuple.c     |   2 +-
>   src/box/lua/tuple.h     |   3 ++
>   test/box/tuple.result   |  95 +++++++++++++++++++++++++++++++++++++
>   test/box/tuple.test.lua |  29 ++++++++++++
>   6 files changed, 252 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 30c6bc6..323c2d6 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1419,6 +1419,7 @@ function box.schema.space.bless(space)
>           end
>           builtin.space_run_triggers(s, yesno)
>       end
> +    space_mt.frommap = box.internal.space.frommap
>       space_mt.__index = space_mt
>   
>       setmetatable(space, space_mt)
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 29a9aca..2723baf 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -174,6 +174,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);
> +
> +	/* space._cptr */
> +	lua_pushstring(L, "_cptr");
> +	lua_pushlightuserdata(L, space);
> +	lua_settable(L, i);
> +
>   	lua_pushstring(L, "enabled");
>   	lua_pushboolean(L, space_index(space, 0) != 0);
>   	lua_settable(L, i);
> @@ -386,6 +396,111 @@ 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.
> + * @retval not nil C structure pointer.
> + * @retval nil, err Can not find underlying space, error
> + *         description is returned.
> + */
> +static int
> +lbox_space_ptr_cached(struct lua_State *L)
> +{
> +	if (lua_gettop(L) < 1 || !lua_istable(L, 1))
> +		luaL_error(L, "Usage: space:_cptr()");
> +
> +	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 = NULL;
> +	if (schema_version == global_shema_version) {
> +		lua_getfield(L, 1, "_cptr");
> +		space = (void *)lua_topointer(L, -1);
> +	} else {
> +		lua_getfield(L, 1, "id");
> +		uint32_t id = luaL_touint64(L, -1);
> +		space = space_by_id(id);
> +		if (space == NULL) {
> +			lua_pushnil(L);
> +			lua_pushstring(L, tt_sprintf("Space with id '%d' "\
> +						     "doesn't exist", id));
> +			return 2;
> +		}
> +
> +		/** update the cache */
> +		luaL_pushuint64(L, global_shema_version);
> +		lua_setfield(L, 1, "_schema_version");
> +		lua_pushlightuserdata(L, 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).
> + * @retval not nil A tuple or a table conforming to a space
> + *         format.
> + * @retval nil, err Can not built a tuple. A reason is returned in
> + *         the second value.
> + */
> +static int
> +lbox_space_frommap(struct lua_State *L)
> +{
> +	struct tuple_dictionary *dict = NULL;
> +	struct space *space = NULL;
> +	int rc, argc = lua_gettop(L);
> +	bool table = false;
> +	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
> +		goto usage_error;
> +	if (argc == 3) {
> +		if (!lua_istable(L, 3))
> +			goto usage_error;
> +		lua_getfield(L, 3, "table");
> +		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
> +			goto usage_error;
> +		table = lua_toboolean(L, -1);
> +	}
> +
> +	rc = lbox_space_ptr_cached(L);
> +	if (rc != 1)
> +		return rc;
> +	space = (struct space *) lua_topointer(L, -1);
> +	dict = space->format->dict;
> +	lua_createtable(L, space->def->field_count, 0);
> +
> +	lua_pushnil(L);
> +	while (lua_next(L, 2) != 0) {
> +		uint32_t fieldno;
> +		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)) {
> +			lua_pushnil(L);
> +			lua_pushstring(L, tt_sprintf("Unknown field '%s'",
> +						     key));
> +			return 2;
> +		}
> +		lua_rawseti(L, -3, fieldno+1);
> +	}
> +	if (table)
> +		return 1;
> +
> +	lua_replace(L, 1);
> +	lua_settop(L, 1);
> +	return lbox_tuple_new(L);
> +usage_error:
> +	return luaL_error(L, "Usage: space:frommap(map, opts)");
> +}
> +
>   void
>   box_lua_space_init(struct lua_State *L)
>   {
> @@ -464,4 +579,12 @@ box_lua_space_init(struct lua_State *L)
>   	lua_pushnumber(L, VCLOCK_MAX);
>   	lua_setfield(L, -2, "REPLICA_MAX");
>   	lua_pop(L, 2); /* box, schema */
> +
> +	static const struct luaL_Reg space_internal_lib[] = {
> +		{"frommap", lbox_space_frommap},
> +		{"_cptr", lbox_space_ptr_cached},
> +		{NULL, NULL}
> +	};
> +	luaL_register(L, "box.internal.space", space_internal_lib);
> +	lua_pop(L, 1);
>   }
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 7ca4299..92a7185 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
>   	return *(struct tuple **) data;
>   }
>   
> -static int
> +int
>   lbox_tuple_new(lua_State *L)
>   {
>   	int argc = lua_gettop(L);
> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index 0965644..5d7062e 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
>   
>   /** \endcond public */
>   
> +int
> +lbox_tuple_new(struct lua_State *L);
> +
>   static inline int
>   luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
>   {
> diff --git a/test/box/tuple.result b/test/box/tuple.result
> index ec5ed90..2dd8f0d 100644
> --- a/test/box/tuple.result
> +++ b/test/box/tuple.result
> @@ -1060,6 +1060,101 @@ box.tuple.bsize == t.bsize
>   ---
>   - true
>   ...
> +--
> +-- gh-3282: space:frommap().
> +--
> +format = {}
> +---
> +...
> +format[1] = {name = 'aaa', type = 'unsigned'}
> +---
> +...
> +format[2] = {name = 'bbb', type = 'unsigned'}
> +---
> +...
> +format[3] = {name = 'ccc', type = 'unsigned'}
> +---
> +...
> +format[4] = {name = 'ddd', type = 'unsigned'}
> +---
> +...
> +s = box.schema.create_space('test', {format = format})
> +---
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +- [2, 4, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
> +---
> +- [2, 3, null, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
> +---
> +- null
> +- Unknown field 'eee'
> +...
> +s:frommap()
> +---
> +- error: 'Usage: space:frommap(map, opts)'
> +...
> +s:frommap({})
> +---
> +- []
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> +---
> +- - 2
> +  - 4
> +  - 3
> +  - 1
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
> +---
> +- [2, 4, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> +---
> +- [2, null, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
> +---
> +- [2, 4, 3, 1]
> +...
> +_ = s:create_index('primary', {parts = {'aaa'}})
> +---
> +...
> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +...
> +s:replace(tuple)
> +---
> +- [2, 4, 3, 1]
> +...
> +_, err = box.internal.space._cptr(s)
> +---
> +...
> +assert(not err)
> +---
> +- true
> +...
> +s:drop()
> +---
> +...
> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +...
> +assert(err ~= nil)
> +---
> +- true
> +...
> +_, err = box.internal.space._cptr(s)
> +---
> +...
> +assert(err ~= nil)
> +---
> +- true
> +...
>   test_run:cmd("clear filter")
>   ---
>   - true
> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
> index 0b3392d..03e6194 100644
> --- a/test/box/tuple.test.lua
> +++ b/test/box/tuple.test.lua
> @@ -349,4 +349,33 @@ box.tuple.update == t.update
>   box.tuple.upsert == t.upsert
>   box.tuple.bsize == t.bsize
>   
> +--
> +-- gh-3282: space:frommap().
> +--
> +format = {}
> +format[1] = {name = 'aaa', type = 'unsigned'}
> +format[2] = {name = 'bbb', type = 'unsigned'}
> +format[3] = {name = 'ccc', type = 'unsigned'}
> +format[4] = {name = 'ddd', type = 'unsigned'}
> +s = box.schema.create_space('test', {format = format})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
> +s:frommap()
> +s:frommap({})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
> +_ = s:create_index('primary', {parts = {'aaa'}})
> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +s:replace(tuple)
> +_, err = box.internal.space._cptr(s)
> +assert(not err)
> +s:drop()
> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +assert(err ~= nil)
> +_, err = box.internal.space._cptr(s)
> +assert(err ~= nil)
> +
>   test_run:cmd("clear filter")

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

[Vladimir Davydov]

On Wed, Apr 04, 2018 at 07:35:54PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 29a9aca..2723baf 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -386,6 +396,111 @@ 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.
> + * @retval not nil C structure pointer.
> + * @retval nil, err Can not find underlying space, error
> + *         description is returned.
> + */
> +static int
> +lbox_space_ptr_cached(struct lua_State *L)
> +{

Is it really necessary? Can't you always pass space_id and use
space_cache_find, which already caches last space ptr?

> +	if (lua_gettop(L) < 1 || !lua_istable(L, 1))
> +		luaL_error(L, "Usage: space:_cptr()");
> +
> +	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 = NULL;
> +	if (schema_version == global_shema_version) {
> +		lua_getfield(L, 1, "_cptr");
> +		space = (void *)lua_topointer(L, -1);
> +	} else {
> +		lua_getfield(L, 1, "id");
> +		uint32_t id = luaL_touint64(L, -1);
> +		space = space_by_id(id);
> +		if (space == NULL) {
> +			lua_pushnil(L);
> +			lua_pushstring(L, tt_sprintf("Space with id '%d' "\
> +						     "doesn't exist", id));
> +			return 2;
> +		}
> +
> +		/** update the cache */
> +		luaL_pushuint64(L, global_shema_version);
> +		lua_setfield(L, 1, "_schema_version");
> +		lua_pushlightuserdata(L, space);
> +		lua_setfield(L, 1, "_cptr");
> +	}
> +	lua_pop(L, 1);
> +	lua_pushlightuserdata(L, space);
> +	return 1;
> +}

> @@ -464,4 +579,12 @@ box_lua_space_init(struct lua_State *L)
>  	lua_pushnumber(L, VCLOCK_MAX);
>  	lua_setfield(L, -2, "REPLICA_MAX");
>  	lua_pop(L, 2); /* box, schema */
> +
> +	static const struct luaL_Reg space_internal_lib[] = {
> +		{"frommap", lbox_space_frommap},
> +		{"_cptr", lbox_space_ptr_cached},
> +		{NULL, NULL}
> +	};
> +	luaL_register(L, "box.internal.space", space_internal_lib);
> +	lua_pop(L, 1);

AFAIU this function might get pretty hot. Shouldn't we use FFI for
calling it?

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

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/04/05 10:24]:

> This feature is designed to build tuple or table object by map.
> Tuple output is default. If you wish to make a table, specify table = true option.
> subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})

What are the performance results from adding the cache? 

How much do we win?

It's OK to export _schema_version to Lua as extern int via FFI, 
and avoid a function call.

> +static int
> +lbox_space_ptr_cached(struct lua_State *L)

The fast path of this function should be in plain Lua.

You have to re-fetch the space via lookup in a slowpath only, 
when schema version has changed.


What about using the cptr in all other functions? 


> +{
> +	if (lua_gettop(L) < 1 || !lua_istable(L, 1))
> +		luaL_error(L, "Usage: space:_cptr()");
> +
> +	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 = NULL;
> +	if (schema_version == global_shema_version) {
> +		lua_getfield(L, 1, "_cptr");
> +		space = (void *)lua_topointer(L, -1);
> +	} else {
> +		lua_getfield(L, 1, "id");
> +		uint32_t id = luaL_touint64(L, -1);
> +		space = space_by_id(id);
> +		if (space == NULL) {
> +			lua_pushnil(L);
> +			lua_pushstring(L, tt_sprintf("Space with id '%d' "\
> +						     "doesn't exist", id));
> +			return 2;
> +		}
> +
> +		/** update the cache */
> +		luaL_pushuint64(L, global_shema_version);
> +		lua_setfield(L, 1, "_schema_version");
> +		lua_pushlightuserdata(L, 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).
> + * @retval not nil A tuple or a table conforming to a space
> + *         format.
> + * @retval nil, err Can not built a tuple. A reason is returned in
> + *         the second value.
> + */
> +static int
> +lbox_space_frommap(struct lua_State *L)
> +{
> +	struct tuple_dictionary *dict = NULL;
> +	struct space *space = NULL;
> +	int rc, argc = lua_gettop(L);
> +	bool table = false;
> +	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
> +		goto usage_error;
> +	if (argc == 3) {
> +		if (!lua_istable(L, 3))
> +			goto usage_error;
> +		lua_getfield(L, 3, "table");
> +		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
> +			goto usage_error;
> +		table = lua_toboolean(L, -1);
> +	}
> +
> +	rc = lbox_space_ptr_cached(L);
> +	if (rc != 1)
> +		return rc;
> +	space = (struct space *) lua_topointer(L, -1);
> +	dict = space->format->dict;
> +	lua_createtable(L, space->def->field_count, 0);
> +
> +	lua_pushnil(L);
> +	while (lua_next(L, 2) != 0) {
> +		uint32_t fieldno;
> +		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)) {
> +			lua_pushnil(L);
> +			lua_pushstring(L, tt_sprintf("Unknown field '%s'",
> +						     key));
> +			return 2;
> +		}
> +		lua_rawseti(L, -3, fieldno+1);
> +	}
> +	if (table)
> +		return 1;
> +
> +	lua_replace(L, 1);
> +	lua_settop(L, 1);
> +	return lbox_tuple_new(L);
> +usage_error:
> +	return luaL_error(L, "Usage: space:frommap(map, opts)");
> +}
> +
>  void
>  box_lua_space_init(struct lua_State *L)
>  {
> @@ -464,4 +579,12 @@ box_lua_space_init(struct lua_State *L)
>  	lua_pushnumber(L, VCLOCK_MAX);
>  	lua_setfield(L, -2, "REPLICA_MAX");
>  	lua_pop(L, 2); /* box, schema */
> +
> +	static const struct luaL_Reg space_internal_lib[] = {
> +		{"frommap", lbox_space_frommap},
> +		{"_cptr", lbox_space_ptr_cached},
> +		{NULL, NULL}
> +	};
> +	luaL_register(L, "box.internal.space", space_internal_lib);
> +	lua_pop(L, 1);
>  }
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 7ca4299..92a7185 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
>  	return *(struct tuple **) data;
>  }
>  
> -static int
> +int
>  lbox_tuple_new(lua_State *L)
>  {
>  	int argc = lua_gettop(L);
> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index 0965644..5d7062e 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
>  
>  /** \endcond public */
>  
> +int
> +lbox_tuple_new(struct lua_State *L);
> +
>  static inline int
>  luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
>  {
> diff --git a/test/box/tuple.result b/test/box/tuple.result
> index ec5ed90..2dd8f0d 100644
> --- a/test/box/tuple.result
> +++ b/test/box/tuple.result
> @@ -1060,6 +1060,101 @@ box.tuple.bsize == t.bsize
>  ---
>  - true
>  ...
> +--
> +-- gh-3282: space:frommap().
> +--
> +format = {}
> +---
> +...
> +format[1] = {name = 'aaa', type = 'unsigned'}
> +---
> +...
> +format[2] = {name = 'bbb', type = 'unsigned'}
> +---
> +...
> +format[3] = {name = 'ccc', type = 'unsigned'}
> +---
> +...
> +format[4] = {name = 'ddd', type = 'unsigned'}
> +---
> +...
> +s = box.schema.create_space('test', {format = format})
> +---
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +- [2, 4, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
> +---
> +- [2, 3, null, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
> +---
> +- null
> +- Unknown field 'eee'
> +...
> +s:frommap()
> +---
> +- error: 'Usage: space:frommap(map, opts)'
> +...
> +s:frommap({})
> +---
> +- []
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> +---
> +- - 2
> +  - 4
> +  - 3
> +  - 1
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
> +---
> +- [2, 4, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> +---
> +- [2, null, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
> +---
> +- [2, 4, 3, 1]
> +...
> +_ = s:create_index('primary', {parts = {'aaa'}})
> +---
> +...
> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +...
> +s:replace(tuple)
> +---
> +- [2, 4, 3, 1]
> +...
> +_, err = box.internal.space._cptr(s)
> +---
> +...
> +assert(not err)
> +---
> +- true
> +...
> +s:drop()
> +---
> +...
> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +...
> +assert(err ~= nil)
> +---
> +- true
> +...
> +_, err = box.internal.space._cptr(s)
> +---
> +...
> +assert(err ~= nil)
> +---
> +- true
> +...
>  test_run:cmd("clear filter")
>  ---
>  - true
> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
> index 0b3392d..03e6194 100644
> --- a/test/box/tuple.test.lua
> +++ b/test/box/tuple.test.lua
> @@ -349,4 +349,33 @@ box.tuple.update == t.update
>  box.tuple.upsert == t.upsert
>  box.tuple.bsize == t.bsize
>  
> +--
> +-- gh-3282: space:frommap().
> +--
> +format = {}
> +format[1] = {name = 'aaa', type = 'unsigned'}
> +format[2] = {name = 'bbb', type = 'unsigned'}
> +format[3] = {name = 'ccc', type = 'unsigned'}
> +format[4] = {name = 'ddd', type = 'unsigned'}
> +s = box.schema.create_space('test', {format = format})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
> +s:frommap()
> +s:frommap({})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
> +_ = s:create_index('primary', {parts = {'aaa'}})
> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +s:replace(tuple)
> +_, err = box.internal.space._cptr(s)
> +assert(not err)
> +s:drop()
> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +assert(err ~= nil)
> +_, err = box.internal.space._cptr(s)
> +assert(err ~= nil)
> +
>  test_run:cmd("clear filter")
> -- 
> 2.7.4
> 
> 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

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

[Kirill Shcherbatov]

I've accounted Kostya's comments. 
Going to measure performance.


From ef47d3536b845a84b52be6c84a66aef12fbe8561 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Mon, 9 Apr 2018 11:24:38 +0300
Subject: [PATCH] Moved obtaining cptr to Lua code.

---
 src/box/lua/schema.lua | 33 +++++++++++++++++++++++++---
 src/box/lua/space.cc   | 59 ++++++--------------------------------------------
 2 files changed, 37 insertions(+), 55 deletions(-)

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 323c2d6..a0f2d25 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1072,6 +1072,19 @@ end
 
 internal.check_iterator_type = check_iterator_type -- export for net.box
 
+-- Get a C space structure pointer by space id using cache.
+-- Update cache if necessary.
+-- The _schema_version field is a cache invalidation rule.
+function space_cptr_cached(space)
+    local schema_version = builtin.box_schema_version()
+    if space._schema_version == schema_version then
+        return space._cptr
+    end
+    space._cptr = builtin.space_by_id(space.id)
+    space._schema_version = schema_version
+    return space._cptr
+end
+
 function box.schema.space.bless(space)
     local index_mt = {}
     -- __len and __index
@@ -1320,7 +1333,7 @@ function box.schema.space.bless(space)
     end
     space_mt.bsize = function(space)
         check_space_arg(space, 'bsize')
-        local s = builtin.space_by_id(space.id)
+        local s = space_cptr_cached(space)
         if s == nil then
             box.error(box.error.NO_SUCH_SPACE, space.name)
         end
@@ -1413,13 +1426,20 @@ function box.schema.space.bless(space)
     end
     space_mt.run_triggers = function(space, yesno)
         check_space_arg(space, 'run_triggers')
-        local s = builtin.space_by_id(space.id)
+        local s = space_cptr_cached(space)
         if s == nil then
             box.error(box.error.NO_SUCH_SPACE, space.name)
         end
         builtin.space_run_triggers(s, yesno)
     end
-    space_mt.frommap = box.internal.space.frommap
+    space_mt.frommap = function(space, map, opts)
+        -- update the cache
+        local cptr = space_cptr_cached(space)
+        if cptr == nil then
+            return box.NULL, "Space " .. space.name .. " does not exist"
+        end
+        return box.internal.space.frommap(space, map, opts)
+    end
     space_mt.__index = space_mt
 
     setmetatable(space, space_mt)
@@ -2234,5 +2254,12 @@ box.internal.schema = {}
 box.internal.schema.init = function()
     box_sequence_init()
 end
+box.internal.space._cptr = function(space)
+    local cptr = space_cptr_cached(space)
+    if cptr == nil then
+        return box.NULL, "Space " .. space.name .. " does not exist"
+    end
+    return cptr
+end
 
 box.NULL = msgpack.NULL
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 2723baf..9383096 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -397,51 +397,6 @@ static struct trigger on_alter_space_in_lua = {
 };
 
 /**
- * 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.
- * @retval not nil C structure pointer.
- * @retval nil, err Can not find underlying space, error
- *         description is returned.
- */
-static int
-lbox_space_ptr_cached(struct lua_State *L)
-{
-	if (lua_gettop(L) < 1 || !lua_istable(L, 1))
-		luaL_error(L, "Usage: space:_cptr()");
-
-	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 = NULL;
-	if (schema_version == global_shema_version) {
-		lua_getfield(L, 1, "_cptr");
-		space = (void *)lua_topointer(L, -1);
-	} else {
-		lua_getfield(L, 1, "id");
-		uint32_t id = luaL_touint64(L, -1);
-		space = space_by_id(id);
-		if (space == NULL) {
-			lua_pushnil(L);
-			lua_pushstring(L, tt_sprintf("Space with id '%d' "\
-						     "doesn't exist", id));
-			return 2;
-		}
-
-		/** update the cache */
-		luaL_pushuint64(L, global_shema_version);
-		lua_setfield(L, 1, "_schema_version");
-		lua_pushlightuserdata(L, 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.
@@ -456,11 +411,11 @@ lbox_space_frommap(struct lua_State *L)
 {
 	struct tuple_dictionary *dict = NULL;
 	struct space *space = NULL;
-	int rc, argc = lua_gettop(L);
+	int argc = lua_gettop(L);
 	bool table = false;
 	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
 		goto usage_error;
-	if (argc == 3) {
+	if (argc == 3 && !lua_isnil(L, 3)) {
 		if (!lua_istable(L, 3))
 			goto usage_error;
 		lua_getfield(L, 3, "table");
@@ -469,10 +424,11 @@ lbox_space_frommap(struct lua_State *L)
 		table = lua_toboolean(L, -1);
 	}
 
-	rc = lbox_space_ptr_cached(L);
-	if (rc != 1)
-		return rc;
-	space = (struct space *) lua_topointer(L, -1);
+	lua_getfield(L, 1, "_cptr");
+	space = (struct space *)lua_topointer(L, -1);
+	/* validation is in the Lua code */
+	assert(space);
+
 	dict = space->format->dict;
 	lua_createtable(L, space->def->field_count, 0);
 
@@ -582,7 +538,6 @@ box_lua_space_init(struct lua_State *L)
 
 	static const struct luaL_Reg space_internal_lib[] = {
 		{"frommap", lbox_space_frommap},
-		{"_cptr", lbox_space_ptr_cached},
 		{NULL, NULL}
 	};
 	luaL_register(L, "box.internal.space", space_internal_lib);
-- 
2.7.4

On 06.04.2018 18:44, Konstantin Osipov wrote:
> * Kirill Shcherbatov <kshcherbatov@tarantool.org> [18/04/05 10:24]:
> 
>> This feature is designed to build tuple or table object by map.
>> Tuple output is default. If you wish to make a table, specify table = true option.
>> subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> 
> What are the performance results from adding the cache? 
> 
> How much do we win?
> 
> It's OK to export _schema_version to Lua as extern int via FFI, 
> and avoid a function call.
> 
>> +static int
>> +lbox_space_ptr_cached(struct lua_State *L)
> 
> The fast path of this function should be in plain Lua.
> 
> You have to re-fetch the space via lookup in a slowpath only, 
> when schema version has changed.
> 
> 
> What about using the cptr in all other functions? 
> 
> 
>> +{
>> +	if (lua_gettop(L) < 1 || !lua_istable(L, 1))
>> +		luaL_error(L, "Usage: space:_cptr()");
>> +
>> +	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 = NULL;
>> +	if (schema_version == global_shema_version) {
>> +		lua_getfield(L, 1, "_cptr");
>> +		space = (void *)lua_topointer(L, -1);
>> +	} else {
>> +		lua_getfield(L, 1, "id");
>> +		uint32_t id = luaL_touint64(L, -1);
>> +		space = space_by_id(id);
>> +		if (space == NULL) {
>> +			lua_pushnil(L);
>> +			lua_pushstring(L, tt_sprintf("Space with id '%d' "\
>> +						     "doesn't exist", id));
>> +			return 2;
>> +		}
>> +
>> +		/** update the cache */
>> +		luaL_pushuint64(L, global_shema_version);
>> +		lua_setfield(L, 1, "_schema_version");
>> +		lua_pushlightuserdata(L, 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).
>> + * @retval not nil A tuple or a table conforming to a space
>> + *         format.
>> + * @retval nil, err Can not built a tuple. A reason is returned in
>> + *         the second value.
>> + */
>> +static int
>> +lbox_space_frommap(struct lua_State *L)
>> +{
>> +	struct tuple_dictionary *dict = NULL;
>> +	struct space *space = NULL;
>> +	int rc, argc = lua_gettop(L);
>> +	bool table = false;
>> +	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
>> +		goto usage_error;
>> +	if (argc == 3) {
>> +		if (!lua_istable(L, 3))
>> +			goto usage_error;
>> +		lua_getfield(L, 3, "table");
>> +		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
>> +			goto usage_error;
>> +		table = lua_toboolean(L, -1);
>> +	}
>> +
>> +	rc = lbox_space_ptr_cached(L);
>> +	if (rc != 1)
>> +		return rc;
>> +	space = (struct space *) lua_topointer(L, -1);
>> +	dict = space->format->dict;
>> +	lua_createtable(L, space->def->field_count, 0);
>> +
>> +	lua_pushnil(L);
>> +	while (lua_next(L, 2) != 0) {
>> +		uint32_t fieldno;
>> +		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)) {
>> +			lua_pushnil(L);
>> +			lua_pushstring(L, tt_sprintf("Unknown field '%s'",
>> +						     key));
>> +			return 2;
>> +		}
>> +		lua_rawseti(L, -3, fieldno+1);
>> +	}
>> +	if (table)
>> +		return 1;
>> +
>> +	lua_replace(L, 1);
>> +	lua_settop(L, 1);
>> +	return lbox_tuple_new(L);
>> +usage_error:
>> +	return luaL_error(L, "Usage: space:frommap(map, opts)");
>> +}
>> +
>>  void
>>  box_lua_space_init(struct lua_State *L)
>>  {
>> @@ -464,4 +579,12 @@ box_lua_space_init(struct lua_State *L)
>>  	lua_pushnumber(L, VCLOCK_MAX);
>>  	lua_setfield(L, -2, "REPLICA_MAX");
>>  	lua_pop(L, 2); /* box, schema */
>> +
>> +	static const struct luaL_Reg space_internal_lib[] = {
>> +		{"frommap", lbox_space_frommap},
>> +		{"_cptr", lbox_space_ptr_cached},
>> +		{NULL, NULL}
>> +	};
>> +	luaL_register(L, "box.internal.space", space_internal_lib);
>> +	lua_pop(L, 1);
>>  }
>> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
>> index 7ca4299..92a7185 100644
>> --- a/src/box/lua/tuple.c
>> +++ b/src/box/lua/tuple.c
>> @@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
>>  	return *(struct tuple **) data;
>>  }
>>  
>> -static int
>> +int
>>  lbox_tuple_new(lua_State *L)
>>  {
>>  	int argc = lua_gettop(L);
>> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
>> index 0965644..5d7062e 100644
>> --- a/src/box/lua/tuple.h
>> +++ b/src/box/lua/tuple.h
>> @@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
>>  
>>  /** \endcond public */
>>  
>> +int
>> +lbox_tuple_new(struct lua_State *L);
>> +
>>  static inline int
>>  luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
>>  {
>> diff --git a/test/box/tuple.result b/test/box/tuple.result
>> index ec5ed90..2dd8f0d 100644
>> --- a/test/box/tuple.result
>> +++ b/test/box/tuple.result
>> @@ -1060,6 +1060,101 @@ box.tuple.bsize == t.bsize
>>  ---
>>  - true
>>  ...
>> +--
>> +-- gh-3282: space:frommap().
>> +--
>> +format = {}
>> +---
>> +...
>> +format[1] = {name = 'aaa', type = 'unsigned'}
>> +---
>> +...
>> +format[2] = {name = 'bbb', type = 'unsigned'}
>> +---
>> +...
>> +format[3] = {name = 'ccc', type = 'unsigned'}
>> +---
>> +...
>> +format[4] = {name = 'ddd', type = 'unsigned'}
>> +---
>> +...
>> +s = box.schema.create_space('test', {format = format})
>> +---
>> +...
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
>> +---
>> +- [2, 4, 3, 1]
>> +...
>> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
>> +---
>> +- [2, 3, null, 1]
>> +...
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
>> +---
>> +- null
>> +- Unknown field 'eee'
>> +...
>> +s:frommap()
>> +---
>> +- error: 'Usage: space:frommap(map, opts)'
>> +...
>> +s:frommap({})
>> +---
>> +- []
>> +...
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
>> +---
>> +- - 2
>> +  - 4
>> +  - 3
>> +  - 1
>> +...
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
>> +---
>> +- [2, 4, 3, 1]
>> +...
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
>> +---
>> +- [2, null, 3, 1]
>> +...
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
>> +---
>> +- [2, 4, 3, 1]
>> +...
>> +_ = s:create_index('primary', {parts = {'aaa'}})
>> +---
>> +...
>> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
>> +---
>> +...
>> +s:replace(tuple)
>> +---
>> +- [2, 4, 3, 1]
>> +...
>> +_, err = box.internal.space._cptr(s)
>> +---
>> +...
>> +assert(not err)
>> +---
>> +- true
>> +...
>> +s:drop()
>> +---
>> +...
>> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
>> +---
>> +...
>> +assert(err ~= nil)
>> +---
>> +- true
>> +...
>> +_, err = box.internal.space._cptr(s)
>> +---
>> +...
>> +assert(err ~= nil)
>> +---
>> +- true
>> +...
>>  test_run:cmd("clear filter")
>>  ---
>>  - true
>> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
>> index 0b3392d..03e6194 100644
>> --- a/test/box/tuple.test.lua
>> +++ b/test/box/tuple.test.lua
>> @@ -349,4 +349,33 @@ box.tuple.update == t.update
>>  box.tuple.upsert == t.upsert
>>  box.tuple.bsize == t.bsize
>>  
>> +--
>> +-- gh-3282: space:frommap().
>> +--
>> +format = {}
>> +format[1] = {name = 'aaa', type = 'unsigned'}
>> +format[2] = {name = 'bbb', type = 'unsigned'}
>> +format[3] = {name = 'ccc', type = 'unsigned'}
>> +format[4] = {name = 'ddd', type = 'unsigned'}
>> +s = box.schema.create_space('test', {format = format})
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
>> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
>> +s:frommap()
>> +s:frommap({})
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
>> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
>> +_ = s:create_index('primary', {parts = {'aaa'}})
>> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
>> +s:replace(tuple)
>> +_, err = box.internal.space._cptr(s)
>> +assert(not err)
>> +s:drop()
>> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
>> +assert(err ~= nil)
>> +_, err = box.internal.space._cptr(s)
>> +assert(err ~= nil)
>> +
>>  test_run:cmd("clear filter")
>> -- 
>> 2.7.4
>>
>>
> 

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

[Vladislav Shpilevoy]

Hello. See 7 comments below.

On 09/04/2018 11:30, Kirill Shcherbatov wrote:
> I've accounted Kostya's comments.
> Going to measure performance.
> 
> 
>  From ef47d3536b845a84b52be6c84a66aef12fbe8561 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Mon, 9 Apr 2018 11:24:38 +0300
> Subject: [PATCH] Moved obtaining cptr to Lua code.
> 
> ---
>   src/box/lua/schema.lua | 33 +++++++++++++++++++++++++---
>   src/box/lua/space.cc   | 59 ++++++--------------------------------------------
>   2 files changed, 37 insertions(+), 55 deletions(-)
> 
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 323c2d6..a0f2d25 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1072,6 +1072,19 @@ end
>   
>   internal.check_iterator_type = check_iterator_type -- export for net.box
>   
> +-- Get a C space structure pointer by space id using cache.
> +-- Update cache if necessary.
> +-- The _schema_version field is a cache invalidation rule.
> +function space_cptr_cached(space)
> +    local schema_version = builtin.box_schema_version()
> +    if space._schema_version == schema_version then
> +        return space._cptr
> +    end
> +    space._cptr = builtin.space_by_id(space.id)
> +    space._schema_version = schema_version
> +    return space._cptr
> +end > +
>   function box.schema.space.bless(space)
>       local index_mt = {}
>       -- __len and __index
> @@ -1320,7 +1333,7 @@ function box.schema.space.bless(space)
>       end
>       space_mt.bsize = function(space)
>           check_space_arg(space, 'bsize')
> -        local s = builtin.space_by_id(space.id)
> +        local s = space_cptr_cached(space)

1. I know, it is very small one-line fix, but lets do it in a separate 
patch.

>           if s == nil then
>               box.error(box.error.NO_SUCH_SPACE, space.name)
>           end
> @@ -1413,13 +1426,20 @@ function box.schema.space.bless(space)
>       end
>       space_mt.run_triggers = function(space, yesno)
>           check_space_arg(space, 'run_triggers')
> -        local s = builtin.space_by_id(space.id)
> +        local s = space_cptr_cached(space)

2. This too.

>           if s == nil then
>               box.error(box.error.NO_SUCH_SPACE, space.name)
>           end
>           builtin.space_run_triggers(s, yesno)
>       end
> -    space_mt.frommap = box.internal.space.frommap
> +    space_mt.frommap = function(space, map, opts)
> +        -- update the cache
> +        local cptr = space_cptr_cached(space)
> +        if cptr == nil then
> +            return box.NULL, "Space " .. space.name .. " does not exist"

3. box.NULL == nil, but `if box.NULL` check is true, so box.NULL behaves 
different from nil. Please, return exactly nil.

> +        end
> +        return box.internal.space.frommap(space, map, opts)
> +    end
>       space_mt.__index = space_mt
>   
>       setmetatable(space, space_mt)
> @@ -2234,5 +2254,12 @@ box.internal.schema = {}
>   box.internal.schema.init = function()
>       box_sequence_init()
>   end
> +box.internal.space._cptr = function(space)
> +    local cptr = space_cptr_cached(space)
> +    if cptr == nil then
> +        return box.NULL, "Space " .. space.name .. " does not exist"
> +    end
> +    return cptr
> +end

4. You do not need this function anymore. It is unused.

>   
>   box.NULL = msgpack.NULL
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 2723baf..9383096 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -397,51 +397,6 @@ static struct trigger on_alter_space_in_lua = {
>   };
>   
>   /**
> - * 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.
> - * @retval not nil C structure pointer.
> - * @retval nil, err Can not find underlying space, error
> - *         description is returned.
> - */
> -static int
> -lbox_space_ptr_cached(struct lua_State *L)
> -{
> -	if (lua_gettop(L) < 1 || !lua_istable(L, 1))
> -		luaL_error(L, "Usage: space:_cptr()");
> -
> -	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 = NULL;
> -	if (schema_version == global_shema_version) {
> -		lua_getfield(L, 1, "_cptr");
> -		space = (void *)lua_topointer(L, -1);
> -	} else {
> -		lua_getfield(L, 1, "id");
> -		uint32_t id = luaL_touint64(L, -1);
> -		space = space_by_id(id);
> -		if (space == NULL) {
> -			lua_pushnil(L);
> -			lua_pushstring(L, tt_sprintf("Space with id '%d' "\
> -						     "doesn't exist", id));
> -			return 2;
> -		}
> -
> -		/** update the cache */
> -		luaL_pushuint64(L, global_shema_version);
> -		lua_setfield(L, 1, "_schema_version");
> -		lua_pushlightuserdata(L, 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.
> @@ -456,11 +411,11 @@ lbox_space_frommap(struct lua_State *L)
>   {
>   	struct tuple_dictionary *dict = NULL;
>   	struct space *space = NULL;
> -	int rc, argc = lua_gettop(L);
> +	int argc = lua_gettop(L);
>   	bool table = false;
>   	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
>   		goto usage_error;
> -	if (argc == 3) {
> +	if (argc == 3 && !lua_isnil(L, 3)) {

5. Why do you need this? Error on argc == 3 and argv[3] != nil is the 
same as error on argc > 2.

>   		if (!lua_istable(L, 3))
>   			goto usage_error;
>   		lua_getfield(L, 3, "table");
> @@ -469,10 +424,11 @@ lbox_space_frommap(struct lua_State *L)
>   		table = lua_toboolean(L, -1);
>   	}
>   
> -	rc = lbox_space_ptr_cached(L);
> -	if (rc != 1)
> -		return rc;
> -	space = (struct space *) lua_topointer(L, -1);
> +	lua_getfield(L, 1, "_cptr");
> +	space = (struct space *)lua_topointer(L, -1);

6. Unnecessary diff.

> +	/* validation is in the Lua code */
> +	assert(space);

7. Please, use explicit != NULL, start a sentence with a capital letter, 
and put a dot at the end.

> +
>   	dict = space->format->dict;
>   	lua_createtable(L, space->def->field_count, 0);
>   
> @@ -582,7 +538,6 @@ box_lua_space_init(struct lua_State *L)
>   
>   	static const struct luaL_Reg space_internal_lib[] = {
>   		{"frommap", lbox_space_frommap},
> -		{"_cptr", lbox_space_ptr_cached},
>   		{NULL, NULL}
>   	};
>   	luaL_register(L, "box.internal.space", space_internal_lib);
> 

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

[Kirill Shcherbatov]

From 0cb37cac97056398c39f723aef81053cc4550485 Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Wed, 4 Apr 2018 19:32:40 +0300
Subject: [PATCH] schema:frommap() to create table or tuple

This feature is designed to build tuple or table object by map.
Tuple output is default. If you wish to make a table, specify table = true option.
subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})

Closes #3282
---
 src/box/lua/schema.lua  |  1 +
 src/box/lua/space.cc    | 76 +++++++++++++++++++++++++++++++++++++++++++++-
 src/box/lua/tuple.c     |  2 +-
 src/box/lua/tuple.h     |  3 ++
 test/box/tuple.result   | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
 test/box/tuple.test.lua | 25 +++++++++++++++
 6 files changed, 186 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 30c6bc6..323c2d6 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1419,6 +1419,7 @@ function box.schema.space.bless(space)
         end
         builtin.space_run_triggers(s, yesno)
     end
+    space_mt.frommap = box.internal.space.frommap
     space_mt.__index = space_mt
 
     setmetatable(space, space_mt)
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 29a9aca..e358934 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -178,7 +178,6 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 	lua_pushboolean(L, space_index(space, 0) != 0);
 	lua_settable(L, i);
 
-
         /* space:on_replace */
         lua_pushstring(L, "on_replace");
         lua_pushcfunction(L, lbox_space_on_replace);
@@ -386,6 +385,74 @@ static struct trigger on_alter_space_in_lua = {
 	RLIST_LINK_INITIALIZER, box_lua_space_new_or_delete, NULL, NULL
 };
 
+/**
+ * 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).
+ * @retval not nil A tuple or a table conforming to a space
+ *         format.
+ * @retval nil, err Can not built a tuple. A reason is returned in
+ *         the second value.
+ */
+static int
+lbox_space_frommap(struct lua_State *L)
+{
+	struct tuple_dictionary *dict = NULL;
+	uint32_t id = 0;
+	struct space *space = NULL;
+	int argc = lua_gettop(L);
+	bool table = false;
+	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
+		goto usage_error;
+	if (argc == 3) {
+		if (!lua_istable(L, 3))
+			goto usage_error;
+		lua_getfield(L, 3, "table");
+		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
+			goto usage_error;
+		table = lua_toboolean(L, -1);
+	}
+
+	lua_getfield(L, 1, "id");
+	id = (int)lua_tointeger(L, -1);
+	space = space_by_id(id);
+	if (!space) {
+		lua_pushnil(L);
+		lua_pushstring(L, tt_sprintf("Space with id '%d' "\
+					     "doesn't exist", id));
+		return 2;
+	}
+	assert(space->format != NULL);
+
+	dict = space->format->dict;
+	lua_createtable(L, space->def->field_count, 0);
+
+	lua_pushnil(L);
+	while (lua_next(L, 2) != 0) {
+		uint32_t fieldno;
+		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)) {
+			lua_pushnil(L);
+			lua_pushstring(L, tt_sprintf("Unknown field '%s'",
+						     key));
+			return 2;
+		}
+		lua_rawseti(L, -3, fieldno+1);
+	}
+	if (table)
+		return 1;
+
+	lua_replace(L, 1);
+	lua_settop(L, 1);
+	return lbox_tuple_new(L);
+usage_error:
+	return luaL_error(L, "Usage: space:frommap(map, opts)");
+}
+
 void
 box_lua_space_init(struct lua_State *L)
 {
@@ -464,4 +531,11 @@ box_lua_space_init(struct lua_State *L)
 	lua_pushnumber(L, VCLOCK_MAX);
 	lua_setfield(L, -2, "REPLICA_MAX");
 	lua_pop(L, 2); /* box, schema */
+
+	static const struct luaL_Reg space_internal_lib[] = {
+		{"frommap", lbox_space_frommap},
+		{NULL, NULL}
+	};
+	luaL_register(L, "box.internal.space", space_internal_lib);
+	lua_pop(L, 1);
 }
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 7ca4299..92a7185 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-static int
+int
 lbox_tuple_new(lua_State *L)
 {
 	int argc = lua_gettop(L);
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 0965644..5d7062e 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
 
 /** \endcond public */
 
+int
+lbox_tuple_new(struct lua_State *L);
+
 static inline int
 luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
 {
diff --git a/test/box/tuple.result b/test/box/tuple.result
index ec5ed90..e035cb9 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1060,6 +1060,87 @@ box.tuple.bsize == t.bsize
 ---
 - true
 ...
+--
+-- gh-3282: space:frommap().
+--
+format = {}
+---
+...
+format[1] = {name = 'aaa', type = 'unsigned'}
+---
+...
+format[2] = {name = 'bbb', type = 'unsigned'}
+---
+...
+format[3] = {name = 'ccc', type = 'unsigned'}
+---
+...
+format[4] = {name = 'ddd', type = 'unsigned'}
+---
+...
+s = box.schema.create_space('test', {format = format})
+---
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+- [2, 4, 3, 1]
+...
+s:frommap({ddd = 1, aaa = 2, bbb = 3})
+---
+- [2, 3, null, 1]
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
+---
+- null
+- Unknown field 'eee'
+...
+s:frommap()
+---
+- error: 'Usage: space:frommap(map, opts)'
+...
+s:frommap({})
+---
+- []
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
+---
+- - 2
+  - 4
+  - 3
+  - 1
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
+---
+- [2, 4, 3, 1]
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
+---
+- [2, null, 3, 1]
+...
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
+---
+- [2, 4, 3, 1]
+...
+_ = s:create_index('primary', {parts = {'aaa'}})
+---
+...
+tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+...
+s:replace(tuple)
+---
+- [2, 4, 3, 1]
+...
+s:drop()
+---
+...
+_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+---
+...
+assert(err ~= nil)
+---
+- true
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index 0b3392d..9df978d 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -349,4 +349,29 @@ box.tuple.update == t.update
 box.tuple.upsert == t.upsert
 box.tuple.bsize == t.bsize
 
+--
+-- gh-3282: space:frommap().
+--
+format = {}
+format[1] = {name = 'aaa', type = 'unsigned'}
+format[2] = {name = 'bbb', type = 'unsigned'}
+format[3] = {name = 'ccc', type = 'unsigned'}
+format[4] = {name = 'ddd', type = 'unsigned'}
+s = box.schema.create_space('test', {format = format})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+s:frommap({ddd = 1, aaa = 2, bbb = 3})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
+s:frommap()
+s:frommap({})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
+s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
+_ = s:create_index('primary', {parts = {'aaa'}})
+tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+s:replace(tuple)
+s:drop()
+_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
+assert(err ~= nil)
+
 test_run:cmd("clear filter")
-- 
2.7.4

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

[Vladislav Shpilevoy]

LGTM. Vova, please, take a look.

On 09/04/2018 20:23, Kirill Shcherbatov wrote:
>  From 0cb37cac97056398c39f723aef81053cc4550485 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Wed, 4 Apr 2018 19:32:40 +0300
> Subject: [PATCH] schema:frommap() to create table or tuple
> 
> This feature is designed to build tuple or table object by map.
> Tuple output is default. If you wish to make a table, specify table = true option.
> subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> 
> Closes #3282
> ---
>   src/box/lua/schema.lua  |  1 +
>   src/box/lua/space.cc    | 76 +++++++++++++++++++++++++++++++++++++++++++++-
>   src/box/lua/tuple.c     |  2 +-
>   src/box/lua/tuple.h     |  3 ++
>   test/box/tuple.result   | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>   test/box/tuple.test.lua | 25 +++++++++++++++
>   6 files changed, 186 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 30c6bc6..323c2d6 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -1419,6 +1419,7 @@ function box.schema.space.bless(space)
>           end
>           builtin.space_run_triggers(s, yesno)
>       end
> +    space_mt.frommap = box.internal.space.frommap
>       space_mt.__index = space_mt
>   
>       setmetatable(space, space_mt)
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index 29a9aca..e358934 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -178,7 +178,6 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
>   	lua_pushboolean(L, space_index(space, 0) != 0);
>   	lua_settable(L, i);
>   
> -
>           /* space:on_replace */
>           lua_pushstring(L, "on_replace");
>           lua_pushcfunction(L, lbox_space_on_replace);
> @@ -386,6 +385,74 @@ static struct trigger on_alter_space_in_lua = {
>   	RLIST_LINK_INITIALIZER, box_lua_space_new_or_delete, NULL, NULL
>   };
>   
> +/**
> + * 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).
> + * @retval not nil A tuple or a table conforming to a space
> + *         format.
> + * @retval nil, err Can not built a tuple. A reason is returned in
> + *         the second value.
> + */
> +static int
> +lbox_space_frommap(struct lua_State *L)
> +{
> +	struct tuple_dictionary *dict = NULL;
> +	uint32_t id = 0;
> +	struct space *space = NULL;
> +	int argc = lua_gettop(L);
> +	bool table = false;
> +	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
> +		goto usage_error;
> +	if (argc == 3) {
> +		if (!lua_istable(L, 3))
> +			goto usage_error;
> +		lua_getfield(L, 3, "table");
> +		if (!lua_isboolean(L, -1) && !lua_isnil(L, -1))
> +			goto usage_error;
> +		table = lua_toboolean(L, -1);
> +	}
> +
> +	lua_getfield(L, 1, "id");
> +	id = (int)lua_tointeger(L, -1);
> +	space = space_by_id(id);
> +	if (!space) {
> +		lua_pushnil(L);
> +		lua_pushstring(L, tt_sprintf("Space with id '%d' "\
> +					     "doesn't exist", id));
> +		return 2;
> +	}
> +	assert(space->format != NULL);
> +
> +	dict = space->format->dict;
> +	lua_createtable(L, space->def->field_count, 0);
> +
> +	lua_pushnil(L);
> +	while (lua_next(L, 2) != 0) {
> +		uint32_t fieldno;
> +		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)) {
> +			lua_pushnil(L);
> +			lua_pushstring(L, tt_sprintf("Unknown field '%s'",
> +						     key));
> +			return 2;
> +		}
> +		lua_rawseti(L, -3, fieldno+1);
> +	}
> +	if (table)
> +		return 1;
> +
> +	lua_replace(L, 1);
> +	lua_settop(L, 1);
> +	return lbox_tuple_new(L);
> +usage_error:
> +	return luaL_error(L, "Usage: space:frommap(map, opts)");
> +}
> +
>   void
>   box_lua_space_init(struct lua_State *L)
>   {
> @@ -464,4 +531,11 @@ box_lua_space_init(struct lua_State *L)
>   	lua_pushnumber(L, VCLOCK_MAX);
>   	lua_setfield(L, -2, "REPLICA_MAX");
>   	lua_pop(L, 2); /* box, schema */
> +
> +	static const struct luaL_Reg space_internal_lib[] = {
> +		{"frommap", lbox_space_frommap},
> +		{NULL, NULL}
> +	};
> +	luaL_register(L, "box.internal.space", space_internal_lib);
> +	lua_pop(L, 1);
>   }
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 7ca4299..92a7185 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -90,7 +90,7 @@ luaT_istuple(struct lua_State *L, int narg)
>   	return *(struct tuple **) data;
>   }
>   
> -static int
> +int
>   lbox_tuple_new(lua_State *L)
>   {
>   	int argc = lua_gettop(L);
> diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
> index 0965644..5d7062e 100644
> --- a/src/box/lua/tuple.h
> +++ b/src/box/lua/tuple.h
> @@ -66,6 +66,9 @@ luaT_istuple(struct lua_State *L, int idx);
>   
>   /** \endcond public */
>   
> +int
> +lbox_tuple_new(struct lua_State *L);
> +
>   static inline int
>   luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
>   {
> diff --git a/test/box/tuple.result b/test/box/tuple.result
> index ec5ed90..e035cb9 100644
> --- a/test/box/tuple.result
> +++ b/test/box/tuple.result
> @@ -1060,6 +1060,87 @@ box.tuple.bsize == t.bsize
>   ---
>   - true
>   ...
> +--
> +-- gh-3282: space:frommap().
> +--
> +format = {}
> +---
> +...
> +format[1] = {name = 'aaa', type = 'unsigned'}
> +---
> +...
> +format[2] = {name = 'bbb', type = 'unsigned'}
> +---
> +...
> +format[3] = {name = 'ccc', type = 'unsigned'}
> +---
> +...
> +format[4] = {name = 'ddd', type = 'unsigned'}
> +---
> +...
> +s = box.schema.create_space('test', {format = format})
> +---
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +- [2, 4, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
> +---
> +- [2, 3, null, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
> +---
> +- null
> +- Unknown field 'eee'
> +...
> +s:frommap()
> +---
> +- error: 'Usage: space:frommap(map, opts)'
> +...
> +s:frommap({})
> +---
> +- []
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> +---
> +- - 2
> +  - 4
> +  - 3
> +  - 1
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
> +---
> +- [2, 4, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> +---
> +- [2, null, 3, 1]
> +...
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
> +---
> +- [2, 4, 3, 1]
> +...
> +_ = s:create_index('primary', {parts = {'aaa'}})
> +---
> +...
> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +...
> +s:replace(tuple)
> +---
> +- [2, 4, 3, 1]
> +...
> +s:drop()
> +---
> +...
> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +---
> +...
> +assert(err ~= nil)
> +---
> +- true
> +...
>   test_run:cmd("clear filter")
>   ---
>   - true
> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
> index 0b3392d..9df978d 100644
> --- a/test/box/tuple.test.lua
> +++ b/test/box/tuple.test.lua
> @@ -349,4 +349,29 @@ box.tuple.update == t.update
>   box.tuple.upsert == t.upsert
>   box.tuple.bsize == t.bsize
>   
> +--
> +-- gh-3282: space:frommap().
> +--
> +format = {}
> +format[1] = {name = 'aaa', type = 'unsigned'}
> +format[2] = {name = 'bbb', type = 'unsigned'}
> +format[3] = {name = 'ccc', type = 'unsigned'}
> +format[4] = {name = 'ddd', type = 'unsigned'}
> +s = box.schema.create_space('test', {format = format})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +s:frommap({ddd = 1, aaa = 2, bbb = 3})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
> +s:frommap()
> +s:frommap({})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> +s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {dummy = true})
> +_ = s:create_index('primary', {parts = {'aaa'}})
> +tuple = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +s:replace(tuple)
> +s:drop()
> +_, err = s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4})
> +assert(err ~= nil)
> +
>   test_run:cmd("clear filter")
> 

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

[Vladimir Davydov]

On Mon, Apr 09, 2018 at 08:23:21PM +0300, Kirill Shcherbatov wrote:
> From 0cb37cac97056398c39f723aef81053cc4550485 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Wed, 4 Apr 2018 19:32:40 +0300
> Subject: [PATCH] schema:frommap() to create table or tuple
> 
> This feature is designed to build tuple or table object by map.
> Tuple output is default. If you wish to make a table, specify table = true option.
> subscriber:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
> 
> Closes #3282
> ---
>  src/box/lua/schema.lua  |  1 +
>  src/box/lua/space.cc    | 76 +++++++++++++++++++++++++++++++++++++++++++++-
>  src/box/lua/tuple.c     |  2 +-
>  src/box/lua/tuple.h     |  3 ++
>  test/box/tuple.result   | 81 +++++++++++++++++++++++++++++++++++++++++++++++++
>  test/box/tuple.test.lua | 25 +++++++++++++++
>  6 files changed, 186 insertions(+), 2 deletions(-)

ACK