[tarantool-patches] [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[tarantool-patches] [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Kirill Shcherbatov]

Functions extract_key, compare, compare_with_key, merge defined
for key_def introduced for LUA key_def object.

Close #4025

@TarantoolBot document
Title: Built-in methods for key_def object
Now it is possible to work with index base object - key
definition in LUA. Available methods:
key = key_def:extract_key(tuple)
key_def:compare(tuple_a, tuple_b)
key_def:compare_with_key(tuple, key), where key is tuple or table
new_key_def = key_def:merge(second_key_def)

Example:
-- Removal values for secondary non-unique index
key_def = require('key_def')
s = box.schema.space.create('test')
pk = s:create_index('pk')
idx = s:create_index('test', {unique=false,
                     parts = {{2, 'number', path = 'a'},
                              {2, 'number', path = 'b'}}})
s:insert{1, {a = 1, b = 1}}
s:insert{2, {a = 1, b = 2}}
pk_def = key_def.new(pk.parts)
for _, tuple in pairs(idx:select({1, nil})) do
    local key = pk_def:extract_key(tuple)
    pk:delete(key)
end
---
 src/box/CMakeLists.txt  |   1 +
 src/box/lua/init.c      |   2 +
 src/box/lua/key_def.c   | 134 ++++++++++++++++++++++++++++++++++++++++
 src/box/lua/key_def.h   |   8 +++
 src/box/lua/key_def.lua |  19 ++++++
 src/box/lua/space.cc    |  28 +--------
 test/box/tuple.result   | 133 +++++++++++++++++++++++++++++++++++++++
 test/box/tuple.test.lua |  39 ++++++++++++
 8 files changed, 338 insertions(+), 26 deletions(-)
 create mode 100644 src/box/lua/key_def.lua

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index f25c21045..906ce22e6 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -12,6 +12,7 @@ lua_source(lua_sources lua/net_box.lua)
 lua_source(lua_sources lua/upgrade.lua)
 lua_source(lua_sources lua/console.lua)
 lua_source(lua_sources lua/xlog.lua)
+lua_source(lua_sources lua/key_def.lua)
 set(bin_sources)
 bin_source(bin_sources bootstrap.snap bootstrap.h)
 
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 69f346414..68ef58909 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -63,6 +63,7 @@
 
 extern char session_lua[],
 	tuple_lua[],
+	key_def_lua[],
 	schema_lua[],
 	load_cfg_lua[],
 	xlog_lua[],
@@ -81,6 +82,7 @@ static const char *lua_sources[] = {
 	"box/console", console_lua,
 	"box/load_cfg", load_cfg_lua,
 	"box/xlog", xlog_lua,
+	"box/key_def", key_def_lua,
 	NULL
 };
 
diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index 813582913..6d2c06afb 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -34,8 +34,10 @@
 #include <lua.h>
 #include <lauxlib.h>
 #include "diag.h"
+#include "tuple.h"
 #include "box/key_def.h"
 #include "box/box.h"
+#include "box/tuple.h"
 #include "box/coll_id_cache.h"
 #include "lua/utils.h"
 #include "box/tuple_format.h" /* TUPLE_INDEX_BASE */
@@ -149,6 +151,28 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part)
 	return 0;
 }
 
+void
+lbox_fill_key_part(struct lua_State *L, const struct key_part *part)
+{
+	lua_newtable(L);
+	lua_pushstring(L, field_type_strs[part->type]);
+	lua_setfield(L, -2, "type");
+	lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE);
+	lua_setfield(L, -2, "fieldno");
+	if (part->path != NULL) {
+		lua_pushlstring(L, part->path, part->path_len);
+		lua_setfield(L, -2, "path");
+	}
+	lua_pushboolean(L, key_part_is_nullable(part));
+	lua_setfield(L, -2, "is_nullable");
+	if (part->coll_id != COLL_NONE) {
+		struct coll_id *coll_id = coll_by_id(part->coll_id);
+		assert(coll_id != NULL);
+		lua_pushstring(L, coll_id->name);
+		lua_setfield(L, -2, "collation");
+	}
+}
+
 struct key_def *
 check_key_def(struct lua_State *L, int idx)
 {
@@ -175,6 +199,103 @@ lbox_key_def_gc(struct lua_State *L)
 	return 0;
 }
 
+static int
+lbox_key_def_extract_key(lua_State *L)
+{
+	struct key_def *key_def;
+	struct tuple *tuple;
+	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL ||
+	   (tuple = luaT_istuple(L, 2)) == NULL)
+		return luaL_error(L, "Usage key_def:extract_key(tuple)");
+
+	uint32_t key_size;
+	char *key = tuple_extract_key(tuple, key_def, &key_size);
+	if (key == NULL)
+		return luaT_error(L);
+
+	struct tuple *ret =
+		box_tuple_new(box_tuple_format_default(), key, key + key_size);
+	if (ret == NULL)
+		return luaT_error(L);
+	luaT_pushtuple(L, ret);
+	return 1;
+}
+
+static int
+lbox_key_def_compare(lua_State *L)
+{
+	struct key_def *key_def;
+	struct tuple *tuple_a, *tuple_b;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL ||
+	   (tuple_a = luaT_istuple(L, 2)) == NULL ||
+	   (tuple_b = luaT_istuple(L, 3)) == NULL)
+		return luaL_error(L, "Usage key_def:compare(tuple_a, tuple_b)");
+
+	int rc = tuple_compare(tuple_a, tuple_b, key_def);
+	lua_pushinteger(L, rc);
+	return 1;
+}
+
+static int
+lbox_key_def_compare_with_key(lua_State *L)
+{
+	struct key_def *key_def;
+	struct tuple *tuple, *key_tuple;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL ||
+	   (tuple = luaT_istuple(L, 2)) == NULL)
+		goto usage_error;
+
+	lua_remove(L, 1);
+	lua_remove(L, 1);
+	if (luaT_tuple_new(L, box_tuple_format_default()) != 1 ||
+	    (key_tuple = luaT_istuple(L, -1)) == NULL)
+		goto usage_error;
+
+	const char *key = tuple_data(key_tuple);
+	assert(mp_typeof(*key) == MP_ARRAY);
+	uint32_t part_count = mp_decode_array(&key);
+
+	int rc = tuple_compare_with_key(tuple, key, part_count, key_def);
+	lua_pushinteger(L, rc);
+	return 1;
+usage_error:
+	return luaL_error(L, "Usage key_def:compare_with_key(tuple, key)");
+}
+
+static int
+lbox_key_def_merge(lua_State *L)
+{
+	struct key_def *key_def_a, *key_def_b;
+	if (lua_gettop(L) != 2 || (key_def_a = check_key_def(L, 1)) == NULL ||
+	   (key_def_b = check_key_def(L, 2)) == NULL)
+		return luaL_error(L, "Usage key_def:merge(second_key_def)");
+
+	struct key_def *new_key_def = key_def_merge(key_def_a, key_def_b);
+	if (new_key_def == NULL)
+		return luaT_error(L);
+
+	*(struct key_def **)luaL_pushcdata(L, key_def_type_id) = new_key_def;
+	lua_pushcfunction(L, lbox_key_def_gc);
+	luaL_setcdatagc(L, -2);
+	return 1;
+}
+
+static int
+lbox_key_def_to_map(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 1 || (key_def = check_key_def(L, 1)) == NULL)
+		return luaL_error(L, "Usage key_def:tomap()");
+
+	lua_createtable(L, key_def->part_count, 0);
+	for (uint32_t i = 0; i < key_def->part_count; ++i) {
+		lua_pushnumber(L, i + 1);
+		lbox_fill_key_part(L, &key_def->parts[i]);
+		lua_settable(L, -3);
+	}
+	return 1;
+}
+
 /**
  * Create a new key_def from a Lua table.
  *
@@ -236,5 +357,18 @@ luaopen_key_def(struct lua_State *L)
 	};
 	luaL_register_module(L, "key_def", meta);
 
+	lua_newtable(L); /* key_def.internal */
+	lua_pushcfunction(L, lbox_key_def_extract_key);
+	lua_setfield(L, -2, "extract_key");
+	lua_pushcfunction(L, lbox_key_def_compare);
+	lua_setfield(L, -2, "compare");
+	lua_pushcfunction(L, lbox_key_def_compare_with_key);
+	lua_setfield(L, -2, "compare_with_key");
+	lua_pushcfunction(L, lbox_key_def_merge);
+	lua_setfield(L, -2, "merge");
+	lua_pushcfunction(L, lbox_key_def_to_map);
+	lua_setfield(L, -2, "tomap");
+	lua_setfield(L, -2, "internal");
+
 	return 1;
 }
diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h
index 61894d452..02217d3a3 100644
--- a/src/box/lua/key_def.h
+++ b/src/box/lua/key_def.h
@@ -36,6 +36,14 @@ extern "C" {
 #endif /* defined(__cplusplus) */
 
 struct lua_State;
+struct key_part;
+
+/**
+ * Prepare a new table representing a key_part on top of the Lua
+ * stack.
+ */
+void
+lbox_fill_key_part(struct lua_State *L, const struct key_part *part);
 
 /**
  * Extract a key_def object from a Lua stack.
diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua
new file mode 100644
index 000000000..5dc8d9357
--- /dev/null
+++ b/src/box/lua/key_def.lua
@@ -0,0 +1,19 @@
+local ffi = require('ffi')
+local key_def = require('key_def')
+local key_def_t = ffi.typeof('struct key_def')
+
+local methods = {
+    ['extract_key'] = key_def.internal.extract_key,
+    ['compare'] = key_def.internal.compare,
+    ['compare_with_key'] = key_def.internal.compare_with_key,
+    ['merge'] = key_def.internal.merge,
+    ['tomap'] = key_def.internal.tomap,
+    ['__serialize'] = key_def.internal.tomap,
+}
+
+ffi.metatype(key_def_t, {
+    __index = function(self, key)
+        return methods[key]
+    end,
+    __tostring = function(key_def) return "<struct key_def&>" end,
+})
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 9dfc97b6a..9be27959c 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -30,6 +30,7 @@
  */
 #include "box/lua/space.h"
 #include "box/lua/tuple.h"
+#include "box/lua/key_def.h"
 #include "box/sql/sqlLimit.h"
 #include "lua/utils.h"
 #include "lua/trigger.h"
@@ -286,32 +287,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 
 		for (uint32_t j = 0; j < index_def->key_def->part_count; j++) {
 			lua_pushnumber(L, j + 1);
-			lua_newtable(L);
-			const struct key_part *part =
-				&index_def->key_def->parts[j];
-
-			lua_pushstring(L, field_type_strs[part->type]);
-			lua_setfield(L, -2, "type");
-
-			lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE);
-			lua_setfield(L, -2, "fieldno");
-
-			if (part->path != NULL) {
-				lua_pushlstring(L, part->path, part->path_len);
-				lua_setfield(L, -2, "path");
-			}
-
-			lua_pushboolean(L, key_part_is_nullable(part));
-			lua_setfield(L, -2, "is_nullable");
-
-			if (part->coll_id != COLL_NONE) {
-				struct coll_id *coll_id =
-					coll_by_id(part->coll_id);
-				assert(coll_id != NULL);
-				lua_pushstring(L, coll_id->name);
-				lua_setfield(L, -2, "collation");
-			}
-
+			lbox_fill_key_part(L, &index_def->key_def->parts[j]);
 			lua_settable(L, -3); /* index[k].parts[j] */
 		}
 
diff --git a/test/box/tuple.result b/test/box/tuple.result
index 82ad8404d..a0209ca4a 100644
--- a/test/box/tuple.result
+++ b/test/box/tuple.result
@@ -1231,3 +1231,136 @@ s2:frommap({a="1", k="11"}):tomap({names_only = true})
 s2:drop()
 ---
 ...
+--
+-- gh-4025: Introduce built-in function to get index key
+--          from tuple.
+--
+key_def = require('key_def')
+---
+...
+s = box.schema.space.create('test', {engine='vinyl'})
+---
+...
+pk = s:create_index('pk')
+---
+...
+sk = s:create_index('sk', {unique=false, parts = {{2, 'number'}, {3, 'number'}}})
+---
+...
+tuple_a = box.tuple.new({1, 1, 22})
+---
+...
+tuple_b = box.tuple.new({2, 1, 11})
+---
+...
+tuple_c = box.tuple.new({3, 1, 22})
+---
+...
+pk_def = key_def.new(pk.parts)
+---
+...
+pk_def:extract_key(tuple_a)
+---
+- [1]
+...
+sk_def = key_def.new(sk.parts)
+---
+...
+sk_def:extract_key(tuple_a)
+---
+- [1, 22]
+...
+s:insert(tuple_a)
+---
+- [1, 1, 22]
+...
+s:insert(tuple_b)
+---
+- [2, 1, 11]
+...
+for _, tuple in pairs(sk:select({1, nil})) do pk:delete(pk_def:extract_key(tuple)) end
+---
+...
+s:select()
+---
+- []
+...
+pk_def:compare(tuple_b, tuple_a)
+---
+- 1
+...
+pk_def:compare(tuple_b, tuple_c)
+---
+- -1
+...
+sk_def:compare(tuple_b, tuple_a)
+---
+- -1
+...
+sk_def:compare(tuple_a, tuple_c)
+---
+- 0
+...
+pk_sk_def = pk_def:merge(sk_def)
+---
+...
+pk_sk_def:extract_key(tuple_a)
+---
+- [1, 1, 22]
+...
+pk_sk_def:compare(tuple_a, tuple_b)
+---
+- -1
+...
+sk_pk_def = sk_def:merge(pk_def)
+---
+...
+sk_pk_def:extract_key(tuple_a)
+---
+- [1, 22, 1]
+...
+sk_pk_def:compare(tuple_a, tuple_b)
+---
+- 1
+...
+key = sk_def:extract_key(tuple_a)
+---
+...
+sk_def:compare_with_key(tuple_a, key)
+---
+- 0
+...
+sk_def:compare_with_key(tuple_a, {1, 20})
+---
+- 1
+...
+sk_def:tomap()
+---
+- - type: number
+    is_nullable: false
+    fieldno: 2
+  - type: number
+    is_nullable: false
+    fieldno: 3
+...
+sk_def
+---
+- - type: number
+    is_nullable: false
+    fieldno: 2
+  - type: number
+    is_nullable: false
+    fieldno: 3
+...
+print(sk_def)
+---
+...
+key_def.new(sk_def:tomap())
+---
+- - type: number
+    is_nullable: false
+    fieldno: 2
+  - type: number
+    is_nullable: false
+    fieldno: 3
+...
diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
index 8030b0884..0a626b36a 100644
--- a/test/box/tuple.test.lua
+++ b/test/box/tuple.test.lua
@@ -416,3 +416,42 @@ s2:frommap({a="1", k="11"})
 s2:frommap({a="1", k="11"}):tomap({names_only = true})
 
 s2:drop()
+
+--
+-- gh-4025: Introduce built-in function to get index key
+--          from tuple.
+--
+key_def = require('key_def')
+s = box.schema.space.create('test', {engine='vinyl'})
+pk = s:create_index('pk')
+sk = s:create_index('sk', {unique=false, parts = {{2, 'number'}, {3, 'number'}}})
+tuple_a = box.tuple.new({1, 1, 22})
+tuple_b = box.tuple.new({2, 1, 11})
+tuple_c = box.tuple.new({3, 1, 22})
+pk_def = key_def.new(pk.parts)
+pk_def:extract_key(tuple_a)
+sk_def = key_def.new(sk.parts)
+sk_def:extract_key(tuple_a)
+s:insert(tuple_a)
+s:insert(tuple_b)
+for _, tuple in pairs(sk:select({1, nil})) do pk:delete(pk_def:extract_key(tuple)) end
+s:select()
+pk_def:compare(tuple_b, tuple_a)
+pk_def:compare(tuple_b, tuple_c)
+sk_def:compare(tuple_b, tuple_a)
+sk_def:compare(tuple_a, tuple_c)
+
+pk_sk_def = pk_def:merge(sk_def)
+pk_sk_def:extract_key(tuple_a)
+pk_sk_def:compare(tuple_a, tuple_b)
+sk_pk_def = sk_def:merge(pk_def)
+sk_pk_def:extract_key(tuple_a)
+sk_pk_def:compare(tuple_a, tuple_b)
+
+key = sk_def:extract_key(tuple_a)
+sk_def:compare_with_key(tuple_a, key)
+sk_def:compare_with_key(tuple_a, {1, 20})
+sk_def:tomap()
+sk_def
+print(sk_def)
+key_def.new(sk_def:tomap())
-- 
2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Alexander Turenko]

You don't include the fixup commit, so I'll paste it here and comment:

> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
> index 36c469526..f819bfed9 100644
> --- a/src/box/lua/key_def.c
> +++ b/src/box/lua/key_def.c
> @@ -33,6 +33,7 @@
>  
>  #include <lua.h>
>  #include <lauxlib.h>
> +#include "fiber.h"
>  #include "diag.h"
>  #include "box/key_def.h"
>  #include "box/box.h"
> @@ -94,11 +95,11 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part)
>         /* Set part->is_nullable and part->nullable_action. */
>         lua_pushstring(L, "is_nullable");
>         lua_gettable(L, -2);
> -       if (lua_isnil(L, -1)) {
> +       if (lua_isnil(L, -1) || lua_toboolean(L, -1) == false) {

lua_toboolean() returns int, so it would be better to compare it with 0
explicitly. BTW, nice catch.

>                 part->is_nullable = false;
>                 part->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
>         } else {
> -               part->is_nullable = lua_toboolean(L, -1);
> +               part->is_nullable = true;
>                 part->nullable_action = ON_CONFLICT_ACTION_NONE;
>         }
>         lua_pop(L, 1);
> @@ -143,9 +144,29 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part)
>         /* Set part->sort_order. */
>         part->sort_order = SORT_ORDER_ASC;
>  
> -       /* Set part->path. */
> -       part->path = NULL;
> -
> +       /* Set part->path for JSON index. */
> +       lua_pushstring(L, "path");
> +       lua_gettable(L, -2);
> +       if (!lua_isnil(L, -1)) {
> +               size_t path_len;
> +               const char *path = lua_tolstring(L, -1, &path_len);
> +               if (json_path_validate(path, path_len, TUPLE_INDEX_BASE) != 0) {
> +                       diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
> +                                0 /* FIXME */ + TUPLE_INDEX_BASE, "invalid path");

FIXME?

> +                       return -1;
> +               }
> +               char *tmp = region_alloc(&fiber()->gc, path_len + 1);

Should we do region_used() in lbox_key_def_new() before the loop and do
region_truncate() after?

Even more, I guess we can don't copy strings here at all: lua will not
collect them until we'll return from key_def.new() and it is enough.

> +               if (tmp == NULL) {
> +                       diag_set(OutOfMemory, path_len + 1, "region", "path");
> +                       return -1;
> +               }
> +               memcpy(tmp, path, path_len);
> +               tmp[path_len] = '\0';
> +               part->path = tmp;
> +       } else {
> +               part->path = NULL;
> +       }
> +       lua_pop(L, 1);
>         return 0;
>  }

On Mon, Mar 25, 2019 at 03:27:41PM +0300, Kirill Shcherbatov wrote:
> Functions extract_key, compare, compare_with_key, merge defined
> for key_def introduced for LUA key_def object.
> 
> Close #4025

See also a list of issues from mine patch, they are closed with this
patchset too.

> +void
> +lbox_fill_key_part(struct lua_State *L, const struct key_part *part)

Such functions are usually named lbox_pushfoo(), in this case I propose
lbox_push_key_part().

> +static int
> +lbox_key_def_extract_key(lua_State *L)
> +{
> +	struct key_def *key_def;
> +	struct tuple *tuple;
> +	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL ||
> +	   (tuple = luaT_istuple(L, 2)) == NULL)
> +		return luaL_error(L, "Usage key_def:extract_key(tuple)");

Nit: Add a colon after 'Usage' (here and below).

> +static int
> +lbox_key_def_compare_with_key(lua_State *L)
> +{
> +	struct key_def *key_def;
> +	struct tuple *tuple, *key_tuple;
> +	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL ||
> +	   (tuple = luaT_istuple(L, 2)) == NULL)
> +		goto usage_error;
> +
> +	lua_remove(L, 1);
> +	lua_remove(L, 1);
> +	if (luaT_tuple_new(L, box_tuple_format_default()) != 1 ||

Maybe extract
https://github.com/tarantool/tarantool/commit/6427413d27dad7cdaaff46a5705ad45b93549275
too and use it here (it is part of Totktonada/gh-3276-on-board-merger patchset?

> +static int
> +lbox_key_def_merge(lua_State *L)
> +{
> +	struct key_def *key_def_a, *key_def_b;
> +	if (lua_gettop(L) != 2 || (key_def_a = check_key_def(L, 1)) == NULL ||
> +	   (key_def_b = check_key_def(L, 2)) == NULL)
> +		return luaL_error(L, "Usage key_def:merge(second_key_def)");
> +
> +	struct key_def *new_key_def = key_def_merge(key_def_a, key_def_b);
> +	if (new_key_def == NULL)
> +		return luaT_error(L);
> +
> +	*(struct key_def **)luaL_pushcdata(L, key_def_type_id) = new_key_def;

Nit: add a whitespace after cast.

> +	lua_pushcfunction(L, lbox_key_def_gc);
> +	luaL_setcdatagc(L, -2);
> +	return 1;
> +}
> +
> +static int
> +lbox_key_def_to_map(struct lua_State *L)

Why tomap? It produce an array as result. Maybe lbox_key_def_to_table()? Or
maybe just lbox_key_def_serialize() like lbox_fiber_serialize() (and don't
expose it as :tomap() / :totable()).

> +	lua_createtable(L, key_def->part_count, 0);
> +	for (uint32_t i = 0; i < key_def->part_count; ++i) {
> +		lua_pushnumber(L, i + 1);
> +		lbox_fill_key_part(L, &key_def->parts[i]);
> +		lua_settable(L, -3);

lua_rawgei() is more convenient here.

> +/**
> + * Prepare a new table representing a key_part on top of the Lua
> + * stack.
> + */

I would rephrase a bit:

> Push a new table representing a key_part to a Lua stack.

> +ffi.metatype(key_def_t, {
> +    __index = function(self, key)
> +        return methods[key]
> +    end,
> +    __tostring = function(key_def) return "<struct key_def&>" end,

Nit: add a whitespace before the ampersand.

> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
> index 8030b0884..0a626b36a 100644
> --- a/test/box/tuple.test.lua
> +++ b/test/box/tuple.test.lua

There is test/box-tap/key_def.test.lua (introduced in the first path).

> @@ -416,3 +416,42 @@ s2:frommap({a="1", k="11"})
>  s2:frommap({a="1", k="11"}):tomap({names_only = true})
>  
>  s2:drop()
> +
> +--
> +-- gh-4025: Introduce built-in function to get index key
> +--          from tuple.
> +--
> +key_def = require('key_def')
> +s = box.schema.space.create('test', {engine='vinyl'})

Why vinyl? I don't think we even need to create a space for this test.

> +pk = s:create_index('pk')
> +sk = s:create_index('sk', {unique=false, parts = {{2, 'number'}, {3, 'number'}}})
> +tuple_a = box.tuple.new({1, 1, 22})
> +tuple_b = box.tuple.new({2, 1, 11})
> +tuple_c = box.tuple.new({3, 1, 22})
> +pk_def = key_def.new(pk.parts)
> +pk_def:extract_key(tuple_a)
> +sk_def = key_def.new(sk.parts)
> +sk_def:extract_key(tuple_a)
> +s:insert(tuple_a)
> +s:insert(tuple_b)
> +for _, tuple in pairs(sk:select({1, nil})) do pk:delete(pk_def:extract_key(tuple)) end
> +s:select()
> +pk_def:compare(tuple_b, tuple_a)
> +pk_def:compare(tuple_b, tuple_c)
> +sk_def:compare(tuple_b, tuple_a)
> +sk_def:compare(tuple_a, tuple_c)
> +
> +pk_sk_def = pk_def:merge(sk_def)
> +pk_sk_def:extract_key(tuple_a)
> +pk_sk_def:compare(tuple_a, tuple_b)
> +sk_pk_def = sk_def:merge(pk_def)
> +sk_pk_def:extract_key(tuple_a)
> +sk_pk_def:compare(tuple_a, tuple_b)
> +
> +key = sk_def:extract_key(tuple_a)
> +sk_def:compare_with_key(tuple_a, key)
> +sk_def:compare_with_key(tuple_a, {1, 20})
> +sk_def:tomap()
> +sk_def
> +print(sk_def)
> +key_def.new(sk_def:tomap())
> -- 
> 2.21.0
> 

[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Kirill Shcherbatov]

Hi! Thank you for review.
https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods
https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110

Pathes for 2.1.1

Fixup for your commit:
=============================================================
---
 src/box/lua/key_def.c         | 48 +++++++++++++++++++++++++++--------
 test/box-tap/key_def.test.lua | 11 ++++++++
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index 36c469526..38a9e80a7 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -33,6 +33,7 @@
 
 #include <lua.h>
 #include <lauxlib.h>
+#include "fiber.h"
 #include "diag.h"
 #include "box/key_def.h"
 #include "box/box.h"
@@ -48,8 +49,10 @@ static uint32_t key_def_type_id = 0;
  * When successful return 0, otherwise return -1 and set a diag.
  */
 static int
-luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part)
+luaT_key_def_set_part(struct lua_State *L, struct key_part_def *parts,
+		      int part_idx, struct region *region)
 {
+	struct key_part_def *part = &parts[part_idx];
 	/* Set part->fieldno. */
 	lua_pushstring(L, "fieldno");
 	lua_gettable(L, -2);
@@ -94,11 +97,11 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part)
 	/* Set part->is_nullable and part->nullable_action. */
 	lua_pushstring(L, "is_nullable");
 	lua_gettable(L, -2);
-	if (lua_isnil(L, -1)) {
+	if (lua_isnil(L, -1) || lua_toboolean(L, -1) == 0) {
 		part->is_nullable = false;
 		part->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
 	} else {
-		part->is_nullable = lua_toboolean(L, -1);
+		part->is_nullable = true;
 		part->nullable_action = ON_CONFLICT_ACTION_NONE;
 	}
 	lua_pop(L, 1);
@@ -143,9 +146,29 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part)
 	/* Set part->sort_order. */
 	part->sort_order = SORT_ORDER_ASC;
 
-	/* Set part->path. */
-	part->path = NULL;
-
+	/* Set part->path for JSON index. */
+	lua_pushstring(L, "path");
+	lua_gettable(L, -2);
+	if (!lua_isnil(L, -1)) {
+		size_t path_len;
+		const char *path = lua_tolstring(L, -1, &path_len);
+		if (json_path_validate(path, path_len, TUPLE_INDEX_BASE) != 0) {
+			diag_set(ClientError, ER_WRONG_INDEX_OPTIONS,
+				 part_idx + TUPLE_INDEX_BASE, "invalid path");
+			return -1;
+		}
+		char *tmp = region_alloc(region, path_len + 1);
+		if (tmp == NULL) {
+			diag_set(OutOfMemory, path_len + 1, "region", "path");
+			return -1;
+		}
+		memcpy(tmp, path, path_len);
+		tmp[path_len] = '\0';
+		part->path = tmp;
+	} else {
+		part->path = NULL;
+	}
+	lua_pop(L, 1);
 	return 0;
 }
 
@@ -191,28 +214,31 @@ lbox_key_def_new(struct lua_State *L)
 		return luaL_error(L, "Bad params, use: key_def.new({"
 				  "{fieldno = fieldno, type = type"
 				  "[, is_nullable = <boolean>]"
+				  "[, path = <string>]"
 				  "[, collation_id = <number>]"
 				  "[, collation = <string>]}, ...}");
 
 	uint32_t part_count = lua_objlen(L, 1);
 	const ssize_t parts_size = sizeof(struct key_part_def) * part_count;
-	struct key_part_def *parts = malloc(parts_size);
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	struct key_part_def *parts = region_alloc(region, parts_size);
 	if (parts == NULL) {
-		diag_set(OutOfMemory, parts_size, "malloc", "parts");
+		diag_set(OutOfMemory, parts_size, "region", "parts");
 		return luaT_error(L);
 	}
 
 	for (uint32_t i = 0; i < part_count; ++i) {
 		lua_pushinteger(L, i + 1);
 		lua_gettable(L, 1);
-		if (luaT_key_def_set_part(L, &parts[i]) != 0) {
-			free(parts);
+		if (luaT_key_def_set_part(L, parts, i, region) != 0) {
+			region_truncate(region, region_svp);
 			return luaT_error(L);
 		}
 	}
 
 	struct key_def *key_def = key_def_new(parts, part_count);
-	free(parts);
+	region_truncate(region, region_svp);
 	if (key_def == NULL)
 		return luaT_error(L);
 
diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
index 7e6e0e330..fd5d60b18 100755
--- a/test/box-tap/key_def.test.lua
+++ b/test/box-tap/key_def.test.lua
@@ -7,6 +7,7 @@ local key_def = require('key_def')
 local usage_error = 'Bad params, use: key_def.new({' ..
                     '{fieldno = fieldno, type = type' ..
                     '[, is_nullable = <boolean>]' ..
+                    '[, path = <string>]' ..
                     '[, collation_id = <number>]' ..
                     '[, collation = <string>]}, ...}'
 
@@ -95,6 +96,15 @@ local cases = {
         params = {{}, {}},
         exp_err = usage_error,
     },
+    {
+        'Invalid JSON path',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            path = "[3[",
+        }},
+        exp_err = "Wrong index options (field 1): invalid path",
+    },
     {
         'Success case; zero parts',
         parts = {},
@@ -105,6 +115,7 @@ local cases = {
         parts = {
             fieldno = 1,
             type = 'string',
+            path = "[3]",
         },
         exp_err = nil,
     },
-- 
2.21.0


=============================================================
Main patch:
=============================================================
Functions extract_key, compare, compare_with_key, merge defined
for key_def introduced for LUA key_def object.

Closes #4025
Closes #3398

@TarantoolBot document
Title: Built-in methods for key_def object
Now it is possible to work with index base object - key
definition in LUA. Available methods:
key = key_def:extract_key(tuple)
key_def:compare(tuple_a, tuple_b)
key_def:compare_with_key(tuple, key), where key is tuple or table
new_key_def = key_def:merge(second_key_def)

Example:
-- Removal values for secondary non-unique index
key_def = require('key_def')
s = box.schema.space.create('test')
pk = s:create_index('pk')
idx = s:create_index('test', {unique=false,
                     parts = {{2, 'number', path = 'a'},
                              {2, 'number', path = 'b'}}})
s:insert{1, {a = 1, b = 1}}
s:insert{2, {a = 1, b = 2}}
pk_def = key_def.new(pk.parts)
for _, tuple in pairs(idx:select({1, nil})) do
    local key = pk_def:extract_key(tuple)
    pk:delete(key)
end
---
 src/box/CMakeLists.txt        |   1 +
 src/box/lua/init.c            |   2 +
 src/box/lua/key_def.c         | 133 ++++++++++++++++++++++++++++++++++
 src/box/lua/key_def.h         |   7 ++
 src/box/lua/key_def.lua       |  19 +++++
 src/box/lua/space.cc          |  28 +------
 test/box-tap/key_def.test.lua |  43 ++++++++++-
 7 files changed, 206 insertions(+), 27 deletions(-)
 create mode 100644 src/box/lua/key_def.lua

diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index f25c21045..906ce22e6 100644
--- a/src/box/CMakeLists.txt
+++ b/src/box/CMakeLists.txt
@@ -12,6 +12,7 @@ lua_source(lua_sources lua/net_box.lua)
 lua_source(lua_sources lua/upgrade.lua)
 lua_source(lua_sources lua/console.lua)
 lua_source(lua_sources lua/xlog.lua)
+lua_source(lua_sources lua/key_def.lua)
 set(bin_sources)
 bin_source(bin_sources bootstrap.snap bootstrap.h)
 
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 69f346414..68ef58909 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -63,6 +63,7 @@
 
 extern char session_lua[],
 	tuple_lua[],
+	key_def_lua[],
 	schema_lua[],
 	load_cfg_lua[],
 	xlog_lua[],
@@ -81,6 +82,7 @@ static const char *lua_sources[] = {
 	"box/console", console_lua,
 	"box/load_cfg", load_cfg_lua,
 	"box/xlog", xlog_lua,
+	"box/key_def", key_def_lua,
 	NULL
 };
 
diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index 38a9e80a7..c2eb3c071 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -35,8 +35,10 @@
 #include <lauxlib.h>
 #include "fiber.h"
 #include "diag.h"
+#include "tuple.h"
 #include "box/key_def.h"
 #include "box/box.h"
+#include "box/tuple.h"
 #include "box/coll_id_cache.h"
 #include "lua/utils.h"
 #include "box/tuple_format.h" /* TUPLE_INDEX_BASE */
@@ -172,6 +174,28 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *parts,
 	return 0;
 }
 
+void
+lbox_push_key_part(struct lua_State *L, const struct key_part *part)
+{
+	lua_newtable(L);
+	lua_pushstring(L, field_type_strs[part->type]);
+	lua_setfield(L, -2, "type");
+	lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE);
+	lua_setfield(L, -2, "fieldno");
+	if (part->path != NULL) {
+		lua_pushlstring(L, part->path, part->path_len);
+		lua_setfield(L, -2, "path");
+	}
+	lua_pushboolean(L, key_part_is_nullable(part));
+	lua_setfield(L, -2, "is_nullable");
+	if (part->coll_id != COLL_NONE) {
+		struct coll_id *coll_id = coll_by_id(part->coll_id);
+		assert(coll_id != NULL);
+		lua_pushstring(L, coll_id->name);
+		lua_setfield(L, -2, "collation");
+	}
+}
+
 struct key_def *
 check_key_def(struct lua_State *L, int idx)
 {
@@ -198,6 +222,102 @@ lbox_key_def_gc(struct lua_State *L)
 	return 0;
 }
 
+static int
+lbox_key_def_extract_key(lua_State *L)
+{
+	struct key_def *key_def;
+	struct tuple *tuple;
+	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL ||
+	   (tuple = luaT_istuple(L, 2)) == NULL)
+		return luaL_error(L, "Usage: key_def:extract_key(tuple)");
+
+	uint32_t key_size;
+	char *key = tuple_extract_key(tuple, key_def, &key_size);
+	if (key == NULL)
+		return luaT_error(L);
+
+	struct tuple *ret =
+		box_tuple_new(box_tuple_format_default(), key, key + key_size);
+	if (ret == NULL)
+		return luaT_error(L);
+	luaT_pushtuple(L, ret);
+	return 1;
+}
+
+static int
+lbox_key_def_compare(lua_State *L)
+{
+	struct key_def *key_def;
+	struct tuple *tuple_a, *tuple_b;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL ||
+	   (tuple_a = luaT_istuple(L, 2)) == NULL ||
+	   (tuple_b = luaT_istuple(L, 3)) == NULL)
+		return luaL_error(L, "Usage: key_def:compare(tuple_a, tuple_b)");
+
+	int rc = tuple_compare(tuple_a, tuple_b, key_def);
+	lua_pushinteger(L, rc);
+	return 1;
+}
+
+static int
+lbox_key_def_compare_with_key(lua_State *L)
+{
+	struct key_def *key_def;
+	struct tuple *tuple, *key_tuple;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL ||
+	   (tuple = luaT_istuple(L, 2)) == NULL)
+		goto usage_error;
+
+	lua_remove(L, 1);
+	lua_remove(L, 1);
+	if (luaT_tuple_new(L, box_tuple_format_default()) != 1 ||
+	    (key_tuple = luaT_istuple(L, -1)) == NULL)
+		goto usage_error;
+
+	const char *key = tuple_data(key_tuple);
+	assert(mp_typeof(*key) == MP_ARRAY);
+	uint32_t part_count = mp_decode_array(&key);
+
+	int rc = tuple_compare_with_key(tuple, key, part_count, key_def);
+	lua_pushinteger(L, rc);
+	return 1;
+usage_error:
+	return luaL_error(L, "Usage: key_def:compare_with_key(tuple, key)");
+}
+
+static int
+lbox_key_def_merge(lua_State *L)
+{
+	struct key_def *key_def_a, *key_def_b;
+	if (lua_gettop(L) != 2 || (key_def_a = check_key_def(L, 1)) == NULL ||
+	   (key_def_b = check_key_def(L, 2)) == NULL)
+		return luaL_error(L, "Usage: key_def:merge(second_key_def)");
+
+	struct key_def *new_key_def = key_def_merge(key_def_a, key_def_b);
+	if (new_key_def == NULL)
+		return luaT_error(L);
+
+	*(struct key_def **) luaL_pushcdata(L, key_def_type_id) = new_key_def;
+	lua_pushcfunction(L, lbox_key_def_gc);
+	luaL_setcdatagc(L, -2);
+	return 1;
+}
+
+static int
+lbox_key_def_to_table(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 1 || (key_def = check_key_def(L, 1)) == NULL)
+		return luaL_error(L, "Usage: key_def:to_table()");
+
+	lua_createtable(L, key_def->part_count, 0);
+	for (uint32_t i = 0; i < key_def->part_count; ++i) {
+		lbox_push_key_part(L, &key_def->parts[i]);
+		lua_rawseti(L, -2, i + 1);
+	}
+	return 1;
+}
+
 /**
  * Create a new key_def from a Lua table.
  *
@@ -262,5 +382,18 @@ luaopen_key_def(struct lua_State *L)
 	};
 	luaL_register_module(L, "key_def", meta);
 
+	lua_newtable(L); /* key_def.internal */
+	lua_pushcfunction(L, lbox_key_def_extract_key);
+	lua_setfield(L, -2, "extract_key");
+	lua_pushcfunction(L, lbox_key_def_compare);
+	lua_setfield(L, -2, "compare");
+	lua_pushcfunction(L, lbox_key_def_compare_with_key);
+	lua_setfield(L, -2, "compare_with_key");
+	lua_pushcfunction(L, lbox_key_def_merge);
+	lua_setfield(L, -2, "merge");
+	lua_pushcfunction(L, lbox_key_def_to_table);
+	lua_setfield(L, -2, "to_table");
+	lua_setfield(L, -2, "internal");
+
 	return 1;
 }
diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h
index 61894d452..7dc7158e8 100644
--- a/src/box/lua/key_def.h
+++ b/src/box/lua/key_def.h
@@ -36,6 +36,13 @@ extern "C" {
 #endif /* defined(__cplusplus) */
 
 struct lua_State;
+struct key_part;
+
+/**
+ * Push a new table representing a key_part to a Lua stack.
+ */
+void
+lbox_push_key_part(struct lua_State *L, const struct key_part *part);
 
 /**
  * Extract a key_def object from a Lua stack.
diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua
new file mode 100644
index 000000000..1bf76e528
--- /dev/null
+++ b/src/box/lua/key_def.lua
@@ -0,0 +1,19 @@
+local ffi = require('ffi')
+local key_def = require('key_def')
+local key_def_t = ffi.typeof('struct key_def')
+
+local methods = {
+    ['extract_key'] = key_def.internal.extract_key,
+    ['compare'] = key_def.internal.compare,
+    ['compare_with_key'] = key_def.internal.compare_with_key,
+    ['merge'] = key_def.internal.merge,
+    ['to_table'] = key_def.internal.to_table,
+    ['__serialize'] = key_def.internal.to_table,
+}
+
+ffi.metatype(key_def_t, {
+    __index = function(self, key)
+        return methods[key]
+    end,
+    __tostring = function(key_def) return "<struct key_def &>" end,
+})
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 9dfc97b6a..dbcdc9300 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -30,6 +30,7 @@
  */
 #include "box/lua/space.h"
 #include "box/lua/tuple.h"
+#include "box/lua/key_def.h"
 #include "box/sql/sqlLimit.h"
 #include "lua/utils.h"
 #include "lua/trigger.h"
@@ -286,32 +287,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 
 		for (uint32_t j = 0; j < index_def->key_def->part_count; j++) {
 			lua_pushnumber(L, j + 1);
-			lua_newtable(L);
-			const struct key_part *part =
-				&index_def->key_def->parts[j];
-
-			lua_pushstring(L, field_type_strs[part->type]);
-			lua_setfield(L, -2, "type");
-
-			lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE);
-			lua_setfield(L, -2, "fieldno");
-
-			if (part->path != NULL) {
-				lua_pushlstring(L, part->path, part->path_len);
-				lua_setfield(L, -2, "path");
-			}
-
-			lua_pushboolean(L, key_part_is_nullable(part));
-			lua_setfield(L, -2, "is_nullable");
-
-			if (part->coll_id != COLL_NONE) {
-				struct coll_id *coll_id =
-					coll_by_id(part->coll_id);
-				assert(coll_id != NULL);
-				lua_pushstring(L, coll_id->name);
-				lua_setfield(L, -2, "collation");
-			}
-
+			lbox_push_key_part(L, &index_def->key_def->parts[j]);
 			lua_settable(L, -3); /* index[k].parts[j] */
 		}
 
diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
index fd5d60b18..c22ef62a8 100755
--- a/test/box-tap/key_def.test.lua
+++ b/test/box-tap/key_def.test.lua
@@ -123,7 +123,7 @@ local cases = {
 
 local test = tap.test('key_def')
 
-test:plan(#cases - 1)
+test:plan(#cases - 1 + 16)
 for _, case in ipairs(cases) do
     if type(case) == 'function' then
         case()
@@ -145,4 +145,45 @@ for _, case in ipairs(cases) do
     end
 end
 
+--
+-- gh-4025: Introduce built-in function to get index key
+--          from tuple.
+-- gh-3398: Access to index compare function from lua.
+--
+tuple_a = box.tuple.new({1, 1, 22})
+tuple_b = box.tuple.new({2, 1, 11})
+tuple_c = box.tuple.new({3, 1, 22})
+pk_parts = {{type = 'unsigned', fieldno = 1}}
+sk_parts = {{type = 'number', fieldno=2}, {type='number', fieldno=3}}
+pk_def = key_def.new(pk_parts)
+test:is_deeply(pk_def:extract_key(tuple_a):totable(), {1}, "pk extract")
+sk_def = key_def.new(sk_parts)
+test:is_deeply(sk_def:extract_key(tuple_a):totable(), {1, 22}, "sk extract")
+
+test:is(pk_def:compare(tuple_b, tuple_a), 1, "pk compare 1")
+test:is(pk_def:compare(tuple_b, tuple_c), -1, "pk compare 2")
+test:is(sk_def:compare(tuple_b, tuple_a), -1, "sk compare 1")
+test:is(sk_def:compare(tuple_a, tuple_c), 0, "sk compare 1")
+
+pk_sk_def = pk_def:merge(sk_def)
+test:ok(pk_sk_def, "pksk merge")
+test:is_deeply(pk_sk_def:extract_key(tuple_a):totable(), {1, 1, 22},
+               "pksk compare 1")
+test:is(pk_sk_def:compare(tuple_a, tuple_b), -1, "pksk compare 2")
+sk_pk_def = sk_def:merge(pk_def)
+test:ok(pk_sk_def, "skpk merge")
+test:is_deeply(sk_pk_def:extract_key(tuple_a):totable(), {1, 22, 1},
+               "skpk compare 1")
+test:is(sk_pk_def:compare(tuple_a, tuple_b), 1, "skpk compare 1")
+
+key = sk_def:extract_key(tuple_a)
+test:is(sk_def:compare_with_key(tuple_a, key), 0,
+        "sk compare_with_key - extracted tuple key")
+test:is(sk_def:compare_with_key(tuple_a, {1, 22}), 0,
+        "sk compare_with_key - table key")
+test:is_deeply(sk_def:to_table(),
+               {{type='number', fieldno=2, is_nullable=false},
+                {type='number', fieldno=3, is_nullable=false}}, "sk to_table")
+test:is(tostring(sk_def) == "<struct key_def &>", true, "tostring(sk_def)")
+
 os.exit(test:check() and 0 or 1)
-- 
2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Alexander Turenko]

I would prefer if you will answer to review comments explicitly.

Re using lua_tolstring(): I understood that lua_tolstring() only
guarantees that a string is valid until it will be popped from a stack
(despite that fact that we know it has another copy on the stack) and so
a region usage looks good.

Just for the record: We discussed voicely that make this patch depending
on the luaT_tuple_new() implementation from the merger patchset is not
worth to do (just saves two lines of code here).

Changes I made (I mostly add them into a separate 'FIXUP' commit to
allow you to look into them if you want; pushed to your branch back):

* Squashed the commits, updated the commit message.
* Renamed :to_table() to totable().
* Use memcpy(tmp, path, path_len + 1) instead of an explicit '\0'
  assignment (lua_tolstring() allows it).
* Use 'struct lua_State' instead of just 'lua_State' to better match
  with our code style.
* Rewrote a test: split it by cases, do some renaming, fixed mine
  parts = {...} to parts = {{...}}, added more cases.
* Found an assertion fault with a new test case:

tarantool: /home/alex/p/tarantool-meta/r/tarantool/src/box/tuple_extract_key.cc:140: char* tuple_extract_key_slowpath(const tuple*, key_def*, uint32_t*) [with bool contains_sequential_parts = false; bool has_optional_parts = false; bool has_json_paths = false; uint32_t = unsigned int]: Assertion `field != NULL' failed.
Aborted

See the first commented test case. I think that we should allow a
fieldno in key_def that is above then a tuple length when the field is
nullable and raise an error otherwise (when it is not nullable). (I
guess that we can do it at least partially by setting
has_optional_parts).

However when I tried with non-nullable part with {fieldno = 2}
:extract_key({'foo'}) gives me {'foo', 80} -- it seems to go over its
memory. But it asserts like the first one if I added {fieldno = 3}.

All that cases are commented now. Can you please elaborate how to
archieve the behaviour I proposed above and whether it is good way to
go, Kirill?

Also I have an idea: maybe add ability to use tables as tuples? We have
to receive tuples in this way from net.box.

WBR, Alexander Turenko.

On Tue, Mar 26, 2019 at 04:37:54PM +0300, Kirill Shcherbatov wrote:
> Hi! Thank you for review.
> https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods
> https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110

[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [19/03/27 09:35]:

Could you please send the final patch for review?

> I would prefer if you will answer to review comments explicitly.
> 
> Re using lua_tolstring(): I understood that lua_tolstring() only
> guarantees that a string is valid until it will be popped from a stack
> (despite that fact that we know it has another copy on the stack) and so
> a region usage looks good.
> 
> Just for the record: We discussed voicely that make this patch depending
> on the luaT_tuple_new() implementation from the merger patchset is not
> worth to do (just saves two lines of code here).
> 
> Changes I made (I mostly add them into a separate 'FIXUP' commit to
> allow you to look into them if you want; pushed to your branch back):
> 
> * Squashed the commits, updated the commit message.
> * Renamed :to_table() to totable().
> * Use memcpy(tmp, path, path_len + 1) instead of an explicit '\0'
>   assignment (lua_tolstring() allows it).
> * Use 'struct lua_State' instead of just 'lua_State' to better match
>   with our code style.
> * Rewrote a test: split it by cases, do some renaming, fixed mine
>   parts = {...} to parts = {{...}}, added more cases.
> * Found an assertion fault with a new test case:
> 
> tarantool: /home/alex/p/tarantool-meta/r/tarantool/src/box/tuple_extract_key.cc:140: char* tuple_extract_key_slowpath(const tuple*, key_def*, uint32_t*) [with bool contains_sequential_parts = false; bool has_optional_parts = false; bool has_json_paths = false; uint32_t = unsigned int]: Assertion `field != NULL' failed.
> Aborted
> 
> See the first commented test case. I think that we should allow a
> fieldno in key_def that is above then a tuple length when the field is
> nullable and raise an error otherwise (when it is not nullable). (I
> guess that we can do it at least partially by setting
> has_optional_parts).
> 
> However when I tried with non-nullable part with {fieldno = 2}
> :extract_key({'foo'}) gives me {'foo', 80} -- it seems to go over its
> memory. But it asserts like the first one if I added {fieldno = 3}.
> 
> All that cases are commented now. Can you please elaborate how to
> archieve the behaviour I proposed above and whether it is good way to
> go, Kirill?
> 
> Also I have an idea: maybe add ability to use tables as tuples? We have
> to receive tuples in this way from net.box.
> 
> WBR, Alexander Turenko.
> 
> On Tue, Mar 26, 2019 at 04:37:54PM +0300, Kirill Shcherbatov wrote:
> > Hi! Thank you for review.
> > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods
> > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110

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

[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Kirill Shcherbatov]

> Could you please send the final patch for review?
Hi! I've already sent it yesterday

https://www.freelists.org/post/tarantool-patches/PATCH-v3-11-lua-add-key-def-lua-module
"[tarantool-patches] [PATCH v3 1/1] lua: add key_def lua module" 22.04.2019, 15:28

I may send it again, if you wish.

[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/03/25 15:29]:
> Functions extract_key, compare, compare_with_key, merge defined
> for key_def introduced for LUA key_def object.
> 
> Close #4025
> 
> @TarantoolBot document
> Title: Built-in methods for key_def object
> Now it is possible to work with index base object - key
> definition in LUA. Available methods:
> key = key_def:extract_key(tuple)
> key_def:compare(tuple_a, tuple_b)
> key_def:compare_with_key(tuple, key), where key is tuple or table
> new_key_def = key_def:merge(second_key_def)

The API mostly looks good to me. I hesitated a bit about
extract_key() building a tuple, not returning a raw msgpack
pointer and part count, like C api, but then I erred on the side
of safety.

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