[tarantool-patches] [PATCH v2 0/2] lua: add key_def lua module

[tarantool-patches] [PATCH v2 0/2] lua: add key_def lua module

[Kirill Shcherbatov]

Introduced new LUA module key_def.
There are several reasons to add this module:
    * Factor out key parts parsing code from the tuples merger (#3276).
    * Support comparing a tuple with a key / a tuple, support merging
      key_defs from Lua (#3398).
    * Support extracting a key from a tuple (#4025).


Changes in version 2:
    - ability to pass tables, not only tuples as arguments
    - perform types and nullablility validations
    - fixed errors

http://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods
https://github.com/tarantool/tarantool/issues/4025

Alexander Turenko (1):
  lua: add luaT_tuple_new()

Kirill Shcherbatov (1):
  lua: add key_def lua module

 src/CMakeLists.txt              |   1 +
 src/box/CMakeLists.txt          |   2 +
 src/box/lua/init.c              |   5 +
 src/box/lua/key_def.c           | 441 ++++++++++++++++++++++++++++++++
 src/box/lua/key_def.h           |  63 +++++
 src/box/lua/key_def.lua         |  19 ++
 src/box/lua/space.cc            |  35 +--
 src/box/lua/tuple.c             |  58 +++--
 src/box/lua/tuple.h             |  15 +-
 src/box/tuple.h                 |  33 +++
 test/box-tap/key_def.test.lua   | 340 ++++++++++++++++++++++++
 test/unit/CMakeLists.txt        |   4 +
 test/unit/luaT_tuple_new.c      | 178 +++++++++++++
 test/unit/luaT_tuple_new.result |  22 ++
 14 files changed, 1167 insertions(+), 49 deletions(-)
 create mode 100644 src/box/lua/key_def.c
 create mode 100644 src/box/lua/key_def.h
 create mode 100644 src/box/lua/key_def.lua
 create mode 100755 test/box-tap/key_def.test.lua
 create mode 100644 test/unit/luaT_tuple_new.c
 create mode 100644 test/unit/luaT_tuple_new.result

-- 
2.21.0

[tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()

[Kirill Shcherbatov]

From: Alexander Turenko <alexander.turenko@tarantool.org>

The function allows to create a tuple with specific tuple format in C
code using a Lua table, another tuple, or objects on a Lua stack.

Needed for #3276, #3398, #4025
---
 src/box/lua/space.cc            |   7 +-
 src/box/lua/tuple.c             |  58 +++++++----
 src/box/lua/tuple.h             |  15 ++-
 test/unit/CMakeLists.txt        |   4 +
 test/unit/luaT_tuple_new.c      | 178 ++++++++++++++++++++++++++++++++
 test/unit/luaT_tuple_new.result |  22 ++++
 6 files changed, 261 insertions(+), 23 deletions(-)
 create mode 100644 test/unit/luaT_tuple_new.c
 create mode 100644 test/unit/luaT_tuple_new.result

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index 9dfc97b6a..ca793e423 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -463,6 +463,7 @@ lbox_space_frommap(struct lua_State *L)
 	struct tuple_dictionary *dict = NULL;
 	uint32_t id = 0;
 	struct space *space = NULL;
+	struct tuple *tuple = NULL;
 	int argc = lua_gettop(L);
 	bool table = false;
 	if (argc < 2 || argc > 3 || !lua_istable(L, 2))
@@ -510,7 +511,11 @@ lbox_space_frommap(struct lua_State *L)
 
 	lua_replace(L, 1);
 	lua_settop(L, 1);
-	return luaT_tuple_new(L, space->format);
+	tuple = luaT_tuple_new(L, -1, space->format);
+	if (tuple == NULL)
+		return luaT_error(L);
+	luaT_pushtuple(L, tuple);
+	return 1;
 usage_error:
 	return luaL_error(L, "Usage: space:frommap(map, opts)");
 }
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 60c1a3999..3f4e3dbf7 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -93,47 +93,65 @@ luaT_istuple(struct lua_State *L, int narg)
 	return *(struct tuple **) data;
 }
 
-int
-luaT_tuple_new(struct lua_State *L, struct tuple_format *format)
+struct tuple *
+luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
 {
-	int argc = lua_gettop(L);
-	if (argc < 1) {
-		lua_newtable(L); /* create an empty tuple */
-		++argc;
+	if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
+		diag_set(IllegalParams, "A tuple or a table expected, got %s",
+			 lua_typename(L, lua_type(L, idx)));
+		return NULL;
 	}
-	struct ibuf *buf = tarantool_lua_ibuf;
 
+	struct ibuf *buf = tarantool_lua_ibuf;
 	ibuf_reset(buf);
 	struct mpstream stream;
 	mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
 		      luamp_error, L);
-
-	if (argc == 1 && (lua_istable(L, 1) || luaT_istuple(L, 1))) {
-		/* New format: box.tuple.new({1, 2, 3}) */
-		luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
-	} else {
-		/* Backward-compatible format: box.tuple.new(1, 2, 3). */
+	if (idx == 0) {
+		/*
+		 * Create the tuple from lua stack
+		 * objects.
+		 */
+		int argc = lua_gettop(L);
 		mpstream_encode_array(&stream, argc);
 		for (int k = 1; k <= argc; ++k) {
 			luamp_encode(L, luaL_msgpack_default, &stream, k);
 		}
+	} else {
+		/* Create the tuple from a Lua table. */
+		luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
 	}
 	mpstream_flush(&stream);
-
 	struct tuple *tuple = box_tuple_new(format, buf->buf,
-					   buf->buf + ibuf_used(buf));
+					    buf->buf + ibuf_used(buf));
 	if (tuple == NULL)
-		return luaT_error(L);
-	/* box_tuple_new() doesn't leak on exception, see public API doc */
-	luaT_pushtuple(L, tuple);
+		return NULL;
 	ibuf_reinit(tarantool_lua_ibuf);
-	return 1;
+	return tuple;
 }
 
 static int
 lbox_tuple_new(lua_State *L)
 {
-	return luaT_tuple_new(L, box_tuple_format_default());
+	int argc = lua_gettop(L);
+	if (argc < 1) {
+		lua_newtable(L); /* create an empty tuple */
+		++argc;
+	}
+	/*
+	 * Use backward-compatible parameters format:
+	 * box.tuple.new(1, 2, 3) (idx == 0), or the new one:
+	 * box.tuple.new({1, 2, 3}) (idx == 1).
+	 */
+	int idx = argc == 1 && (lua_istable(L, 1) ||
+		luaT_istuple(L, 1));
+	box_tuple_format_t *fmt = box_tuple_format_default();
+	struct tuple *tuple = luaT_tuple_new(L, idx, fmt);
+	if (tuple == NULL)
+		return luaT_error(L);
+	/* box_tuple_new() doesn't leak on exception, see public API doc */
+	luaT_pushtuple(L, tuple);
+	return 1;
 }
 
 static int
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index ba7f01f4b..dd3aa0f08 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -42,6 +42,8 @@ typedef struct tuple box_tuple_t;
 struct lua_State;
 struct mpstream;
 struct luaL_serializer;
+struct tuple_format;
+typedef struct tuple_format box_tuple_format_t;
 
 /** \cond public */
 
@@ -67,8 +69,17 @@ luaT_istuple(struct lua_State *L, int idx);
 
 /** \endcond public */
 
-int
-luaT_tuple_new(struct lua_State *L, struct tuple_format *format);
+/**
+ * Create a new tuple with specific format from a Lua table, a
+ * tuple, or objects on the lua stack.
+ *
+ * Set idx to zero to create the new tuple from objects on the lua
+ * stack.
+ *
+ * In case of an error set a diag and return NULL.
+ */
+struct tuple *
+luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format);
 
 static inline int
 luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 6d5704349..2bcb6e0a8 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -140,6 +140,10 @@ add_executable(histogram.test histogram.c)
 target_link_libraries(histogram.test stat unit)
 add_executable(ratelimit.test ratelimit.c)
 target_link_libraries(ratelimit.test unit)
+add_executable(luaT_tuple_new.test luaT_tuple_new.c)
+target_link_libraries(luaT_tuple_new.test unit box server core misc
+    ${CURL_LIBRARIES} ${LIBYAML_LIBRARIES} ${READLINE_LIBRARIES}
+    ${ICU_LIBRARIES} ${LUAJIT_LIBRARIES})
 
 add_executable(say.test say.c)
 target_link_libraries(say.test core unit)
diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c
new file mode 100644
index 000000000..3486ed5d3
--- /dev/null
+++ b/test/unit/luaT_tuple_new.c
@@ -0,0 +1,178 @@
+#include <string.h>           /* strncmp() */
+#include <lua.h>              /* lua_*() */
+#include <lauxlib.h>          /* luaL_*() */
+#include <lualib.h>           /* luaL_openlibs() */
+#include "unit.h"             /* plan, header, footer, is, ok */
+#include "memory.h"           /* memory_init() */
+#include "fiber.h"            /* fiber_init() */
+#include "small/ibuf.h"       /* struct ibuf */
+#include "box/box.h"          /* box_init() */
+#include "box/tuple.h"        /* box_tuple_format_default() */
+#include "lua/msgpack.h"      /* luaopen_msgpack() */
+#include "box/lua/tuple.h"    /* luaT_tuple_new() */
+#include "diag.h"             /* struct error, diag_*() */
+#include "exception.h"        /* type_IllegalParams */
+
+/*
+ * This test checks all usage cases of luaT_tuple_new():
+ *
+ * * Use with idx == 0 and idx != 0.
+ * * Use with default and non-default formats.
+ * * Use a table and a tuple as an input.
+ * * Use with an unexpected lua type as an input.
+ *
+ * The test does not vary an input table/tuple. This is done in
+ * box/tuple.test.lua.
+ */
+
+extern struct ibuf *tarantool_lua_ibuf;
+
+uint32_t
+min_u32(uint32_t a, uint32_t b)
+{
+	return a < b ? a : b;
+}
+
+void
+check_tuple(const struct tuple *tuple, box_tuple_format_t *format,
+	    int retvals, const char *case_name)
+{
+	uint32_t size;
+	const char *data = tuple_data_range(tuple, &size);
+
+	ok(tuple != NULL, "%s: tuple != NULL", case_name);
+	is(tuple->format_id, tuple_format_id(format),
+	   "%s: check tuple format id", case_name);
+	is(size, 4, "%s: check tuple size", case_name);
+	ok(!strncmp(data, "\x93\x01\x02\x03", min_u32(size, 4)),
+	   "%s: check tuple data", case_name);
+	is(retvals, 0, "%s: check retvals count", case_name);
+}
+
+void
+check_error(struct lua_State *L, const struct tuple *tuple, int retvals,
+	    const char *case_name)
+{
+	const char *exp_err = "A tuple or a table expected, got number";
+	is(tuple, NULL, "%s: tuple == NULL", case_name);
+	is(retvals, 0, "%s: check retvals count", case_name);
+	struct error *e = diag_last_error(diag_get());
+	is(e->type, &type_IllegalParams, "%s: check error type", case_name);
+	ok(!strcmp(e->errmsg, exp_err), "%s: check error message", case_name);
+}
+
+int
+test_basic(struct lua_State *L)
+{
+	plan(19);
+	header();
+
+	int top;
+	struct tuple *tuple;
+	box_tuple_format_t *default_format = box_tuple_format_default();
+
+	/*
+	 * Case: a Lua table on idx == -2 as an input.
+	 */
+
+	/* Prepare the Lua stack. */
+	luaL_loadstring(L, "return {1, 2, 3}");
+	lua_call(L, 0, 1);
+	lua_pushnil(L);
+
+	/* Create and check a tuple. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, -2, default_format);
+	check_tuple(tuple, default_format, lua_gettop(L) - top, "table");
+
+	/* Clean up. */
+	lua_pop(L, 2);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: a tuple on idx == -1 as an input.
+	 */
+
+	/* Prepare the Lua stack. */
+	luaT_pushtuple(L, tuple);
+
+	/* Create and check a tuple. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, -1, default_format);
+	check_tuple(tuple, default_format, lua_gettop(L) - top, "tuple");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: elements on the stack (idx == 0) as an input and
+	 * a non-default format.
+	 */
+
+	/* Prepare the Lua stack. */
+	lua_pushinteger(L, 1);
+	lua_pushinteger(L, 2);
+	lua_pushinteger(L, 3);
+
+	/* Create a new format. */
+	struct key_part_def part;
+	part.fieldno = 0;
+	part.type = FIELD_TYPE_INTEGER;
+	part.coll_id = COLL_NONE;
+	part.is_nullable = false;
+	part.nullable_action = ON_CONFLICT_ACTION_DEFAULT;
+	part.sort_order = SORT_ORDER_ASC;
+	part.path = NULL;
+	struct key_def *key_def = key_def_new(&part, 1);
+	box_tuple_format_t *another_format = box_tuple_format_new(&key_def, 1);
+	key_def_delete(key_def);
+
+	/* Create and check a tuple. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, 0, another_format);
+	check_tuple(tuple, another_format, lua_gettop(L) - top, "objects");
+
+	/* Clean up. */
+	tuple_format_delete(another_format);
+	lua_pop(L, 3);
+	assert(lua_gettop(L) == 0);
+
+	/*
+	 * Case: a lua object of an unexpected type.
+	 */
+
+	/* Prepare the Lua stack. */
+	lua_pushinteger(L, 42);
+
+	/* Try to create and check for the error. */
+	top = lua_gettop(L);
+	tuple = luaT_tuple_new(L, -1, default_format);
+	check_error(L, tuple, lua_gettop(L) - top, "unexpected type");
+
+	/* Clean up. */
+	lua_pop(L, 1);
+	assert(lua_gettop(L) == 0);
+
+	footer();
+	return check_plan();
+}
+
+int
+main()
+{
+	memory_init();
+	fiber_init(fiber_c_invoke);
+
+	ibuf_create(tarantool_lua_ibuf, &cord()->slabc, 16000);
+
+	struct lua_State *L = luaL_newstate();
+	luaL_openlibs(L);
+
+	box_init();
+	box_lua_tuple_init(L);
+	luaopen_msgpack(L);
+	lua_pop(L, 1);
+
+	return test_basic(L);
+}
diff --git a/test/unit/luaT_tuple_new.result b/test/unit/luaT_tuple_new.result
new file mode 100644
index 000000000..110aa68c2
--- /dev/null
+++ b/test/unit/luaT_tuple_new.result
@@ -0,0 +1,22 @@
+1..19
+	*** test_basic ***
+ok 1 - table: tuple != NULL
+ok 2 - table: check tuple format id
+ok 3 - table: check tuple size
+ok 4 - table: check tuple data
+ok 5 - table: check retvals count
+ok 6 - tuple: tuple != NULL
+ok 7 - tuple: check tuple format id
+ok 8 - tuple: check tuple size
+ok 9 - tuple: check tuple data
+ok 10 - tuple: check retvals count
+ok 11 - objects: tuple != NULL
+ok 12 - objects: check tuple format id
+ok 13 - objects: check tuple size
+ok 14 - objects: check tuple data
+ok 15 - objects: check retvals count
+ok 16 - unexpected type: tuple == NULL
+ok 17 - unexpected type: check retvals count
+ok 18 - unexpected type: check error type
+ok 19 - unexpected type: check error message
+	*** test_basic: done ***
-- 
2.21.0

[tarantool-patches] Re: [PATCH v2 1/2] lua: add luaT_tuple_new()

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/03/27 17:30]:

Wow, looks like this breaks compatibility and has no ticket in
itself. 

Please file a ticket describing the new features and incompatible
changes so that we can settle on public api changes before
committing to the implementation.


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

[tarantool-patches] Re: [PATCH v2 1/2] lua: add luaT_tuple_new()

[Alexander Turenko]

On Thu, Mar 28, 2019 at 12:01:29PM +0300, Konstantin Osipov wrote:
> * Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/03/27 17:30]:
> 
> Wow, looks like this breaks compatibility and has no ticket in
> itself. 
> 
> Please file a ticket describing the new features and incompatible
> changes so that we can settle on public api changes before
> committing to the implementation.
> 

It is luaT_tuple_new(), not box_tuple_new(). It is not a part of the
public API (I mean we don't export this function).

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

Re: [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()

[Vladimir Davydov]

On Wed, Mar 27, 2019 at 05:29:27PM +0300, Kirill Shcherbatov wrote:
> From: Alexander Turenko <alexander.turenko@tarantool.org>
> 
> The function allows to create a tuple with specific tuple format in C
> code using a Lua table, another tuple, or objects on a Lua stack.
> 
> Needed for #3276, #3398, #4025
> ---
>  src/box/lua/space.cc            |   7 +-
>  src/box/lua/tuple.c             |  58 +++++++----
>  src/box/lua/tuple.h             |  15 ++-
>  test/unit/CMakeLists.txt        |   4 +
>  test/unit/luaT_tuple_new.c      | 178 ++++++++++++++++++++++++++++++++
>  test/unit/luaT_tuple_new.result |  22 ++++
>  6 files changed, 261 insertions(+), 23 deletions(-)
>  create mode 100644 test/unit/luaT_tuple_new.c
>  create mode 100644 test/unit/luaT_tuple_new.result

Pushed to master.

Re: [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()

[Alexander Turenko]

On Wed, Apr 03, 2019 at 09:01:23PM +0300, Vladimir Davydov wrote:
> On Wed, Mar 27, 2019 at 05:29:27PM +0300, Kirill Shcherbatov wrote:
> > From: Alexander Turenko <alexander.turenko@tarantool.org>
> > 
> > The function allows to create a tuple with specific tuple format in C
> > code using a Lua table, another tuple, or objects on a Lua stack.
> > 
> > Needed for #3276, #3398, #4025
> > ---
> >  src/box/lua/space.cc            |   7 +-
> >  src/box/lua/tuple.c             |  58 +++++++----
> >  src/box/lua/tuple.h             |  15 ++-
> >  test/unit/CMakeLists.txt        |   4 +
> >  test/unit/luaT_tuple_new.c      | 178 ++++++++++++++++++++++++++++++++
> >  test/unit/luaT_tuple_new.result |  22 ++++
> >  6 files changed, 261 insertions(+), 23 deletions(-)
> >  create mode 100644 test/unit/luaT_tuple_new.c
> >  create mode 100644 test/unit/luaT_tuple_new.result
> 
> Pushed to master.

Thanks!

Are there any problems with pushing to 2.1 and 1.10? I need this for
merger, which is planned to land into 1.10.

WBR, Alexander Turenko.

Re: [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()

[Vladimir Davydov]

On Thu, Apr 04, 2019 at 05:51:22AM +0300, Alexander Turenko wrote:
> On Wed, Apr 03, 2019 at 09:01:23PM +0300, Vladimir Davydov wrote:
> > On Wed, Mar 27, 2019 at 05:29:27PM +0300, Kirill Shcherbatov wrote:
> > > From: Alexander Turenko <alexander.turenko@tarantool.org>
> > > 
> > > The function allows to create a tuple with specific tuple format in C
> > > code using a Lua table, another tuple, or objects on a Lua stack.
> > > 
> > > Needed for #3276, #3398, #4025
> > > ---
> > >  src/box/lua/space.cc            |   7 +-
> > >  src/box/lua/tuple.c             |  58 +++++++----
> > >  src/box/lua/tuple.h             |  15 ++-
> > >  test/unit/CMakeLists.txt        |   4 +
> > >  test/unit/luaT_tuple_new.c      | 178 ++++++++++++++++++++++++++++++++
> > >  test/unit/luaT_tuple_new.result |  22 ++++
> > >  6 files changed, 261 insertions(+), 23 deletions(-)
> > >  create mode 100644 test/unit/luaT_tuple_new.c
> > >  create mode 100644 test/unit/luaT_tuple_new.result
> > 
> > Pushed to master.
> 
> Thanks!

You're welcome)

> 
> Are there any problems with pushing to 2.1 and 1.10? I need this for
> merger, which is planned to land into 1.10.

Let's think about backporting this patch to 2.1 and 1.10 after we push
the merger to master.

[tarantool-patches] [PATCH v2 2/2] lua: add key_def lua module

[Kirill Shcherbatov]

There are several reasons to add this module:

* Factor out key parts parsing code from the tuples merger (#3276).
* Support comparing a tuple with a key / a tuple, support merging
  key_defs from Lua (#3398).
* Support extracting a key from a tuple (#4025).

The format of `parts` parameter in the `key_def.new(parts)` call is
compatible with the following structures:

* box.space[...].index[...].parts;
* net_box_conn.space[...].index[...].parts.

A key_def instance has the following methods:

* :extract_key(tuple)           -> key (as tuple)
* :compare(tuple_a, tuple_b)    -> number
* :compare_with_key(tuple, key) -> number
* :merge(another_key_def)       -> new key_def instance
* :totable()                    -> table

Note for compare_with_key(): `key` is tuple or table.

Needed for #3276.
Fixes #3398.
Fixes #4025.

@TarantoolBot document
Title: lua: key_def module

See the commit message for an API reference.

Example for extract_key():

```lua
-- Remove values got from a secondary non-unique index.
local key_def_lib = require('key_def')
local s = box.schema.space.create('test')
local pk = s:create_index('pk')
local sk = 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}}
local key_def = key_def_lib.new(pk.parts)
for _, tuple in sk:pairs({1})) do
    local key = key_def:extract_key(tuple)
    pk:delete(key)
end
```
---
 src/CMakeLists.txt            |   1 +
 src/box/CMakeLists.txt        |   2 +
 src/box/lua/init.c            |   5 +
 src/box/lua/key_def.c         | 441 ++++++++++++++++++++++++++++++++++
 src/box/lua/key_def.h         |  63 +++++
 src/box/lua/key_def.lua       |  19 ++
 src/box/lua/space.cc          |  28 +--
 src/box/tuple.h               |  33 +++
 test/box-tap/key_def.test.lua | 340 ++++++++++++++++++++++++++
 9 files changed, 906 insertions(+), 26 deletions(-)
 create mode 100644 src/box/lua/key_def.c
 create mode 100644 src/box/lua/key_def.h
 create mode 100644 src/box/lua/key_def.lua
 create mode 100755 test/box-tap/key_def.test.lua

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 7c2395517..a6a18142b 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -136,6 +136,7 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/lua/string.h
     ${CMAKE_SOURCE_DIR}/src/box/txn.h
     ${CMAKE_SOURCE_DIR}/src/box/key_def.h
+    ${CMAKE_SOURCE_DIR}/src/box/lua/key_def.h
     ${CMAKE_SOURCE_DIR}/src/box/field_def.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 59e91b65a..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)
 
@@ -139,6 +140,7 @@ add_library(box STATIC
     lua/net_box.c
     lua/xlog.c
     lua/sql.c
+    lua/key_def.c
     ${bin_sources})
 
 target_link_libraries(box box_error tuple stat xrow xlog vclock crc32 scramble
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 744b2c895..68ef58909 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -59,9 +59,11 @@
 #include "box/lua/console.h"
 #include "box/lua/tuple.h"
 #include "box/lua/sql.h"
+#include "box/lua/key_def.h"
 
 extern char session_lua[],
 	tuple_lua[],
+	key_def_lua[],
 	schema_lua[],
 	load_cfg_lua[],
 	xlog_lua[],
@@ -80,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
 };
 
@@ -312,6 +315,8 @@ box_lua_init(struct lua_State *L)
 	lua_pop(L, 1);
 	tarantool_lua_console_init(L);
 	lua_pop(L, 1);
+	luaopen_key_def(L);
+	lua_pop(L, 1);
 
 	/* Load Lua extension */
 	for (const char **s = lua_sources; *s; s += 2) {
diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
new file mode 100644
index 000000000..18d5d961d
--- /dev/null
+++ b/src/box/lua/key_def.c
@@ -0,0 +1,441 @@
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "box/lua/key_def.h"
+
+#include <lua.h>
+#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"
+
+static uint32_t key_def_type_id = 0;
+
+/**
+ * Set key_part_def from a table on top of a Lua stack.
+ *
+ * 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 *parts,
+		      int part_idx, struct region *region)
+{
+	struct key_part_def *part = &parts[part_idx];
+	*part = key_part_def_default;
+
+	/* Set part->fieldno. */
+	lua_pushstring(L, "fieldno");
+	lua_gettable(L, -2);
+	if (lua_isnil(L, -1)) {
+		diag_set(IllegalParams, "fieldno must not be nil");
+		return -1;
+	}
+	/*
+	 * Transform one-based Lua fieldno to zero-based
+	 * fieldno to use in key_def_new().
+	 */
+	part->fieldno = lua_tointeger(L, -1) - TUPLE_INDEX_BASE;
+	lua_pop(L, 1);
+
+	/* Set part->type. */
+	lua_pushstring(L, "type");
+	lua_gettable(L, -2);
+	if (lua_isnil(L, -1)) {
+		diag_set(IllegalParams, "type must not be nil");
+		return -1;
+	}
+	size_t type_len;
+	const char *type_name = lua_tolstring(L, -1, &type_len);
+	lua_pop(L, 1);
+	part->type = field_type_by_name(type_name, type_len);
+	switch (part->type) {
+	case FIELD_TYPE_ANY:
+	case FIELD_TYPE_ARRAY:
+	case FIELD_TYPE_MAP:
+		/* Tuple comparators don't support these types. */
+		diag_set(IllegalParams, "Unsupported field type: %s",
+			 type_name);
+		return -1;
+	case field_type_MAX:
+		diag_set(IllegalParams, "Unknown field type: %s", type_name);
+		return -1;
+	default:
+		/* Pass though. */
+		break;
+	}
+
+	/* Set part->is_nullable and part->nullable_action. */
+	lua_pushstring(L, "is_nullable");
+	lua_gettable(L, -2);
+	if (!lua_isnil(L, -1) && lua_toboolean(L, -1) != 0) {
+		part->is_nullable = true;
+		part->nullable_action = ON_CONFLICT_ACTION_NONE;
+	}
+	lua_pop(L, 1);
+
+	/*
+	 * Set part->coll_id using collation_id.
+	 *
+	 * The value will be checked in key_def_new().
+	 */
+	lua_pushstring(L, "collation_id");
+	lua_gettable(L, -2);
+	if (!lua_isnil(L, -1))
+		part->coll_id = lua_tointeger(L, -1);
+	lua_pop(L, 1);
+
+	/* Set part->coll_id using collation. */
+	lua_pushstring(L, "collation");
+	lua_gettable(L, -2);
+	if (!lua_isnil(L, -1)) {
+		/* Check for conflicting options. */
+		if (part->coll_id != COLL_NONE) {
+			diag_set(IllegalParams, "Conflicting options: "
+				 "collation_id and collation");
+			return -1;
+		}
+
+		size_t coll_name_len;
+		const char *coll_name = lua_tolstring(L, -1, &coll_name_len);
+		struct coll_id *coll_id = coll_by_name(coll_name,
+						       coll_name_len);
+		if (coll_id == NULL) {
+			diag_set(IllegalParams, "Unknown collation: \"%s\"",
+				 coll_name);
+			return -1;
+		}
+		part->coll_id = coll_id->id;
+	}
+	lua_pop(L, 1);
+
+	/* Set part->path (JSON path). */
+	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;
+		}
+		/*
+		 * lua_tolstring() guarantees that a string have
+		 * trailing '\0'.
+		 */
+		memcpy(tmp, path, path_len + 1);
+		part->path = tmp;
+	} else {
+		part->path = NULL;
+	}
+	lua_pop(L, 1);
+	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)
+{
+	if (lua_type(L, idx) != LUA_TCDATA)
+		return NULL;
+
+	uint32_t cdata_type;
+	struct key_def **key_def_ptr = luaL_checkcdata(L, idx, &cdata_type);
+	if (key_def_ptr == NULL || cdata_type != key_def_type_id)
+		return NULL;
+	return *key_def_ptr;
+}
+
+/**
+ * Free a key_def from a Lua code.
+ */
+static int
+lbox_key_def_gc(struct lua_State *L)
+{
+	struct key_def *key_def = check_key_def(L, 1);
+	if (key_def == NULL)
+		return 0;
+	box_key_def_delete(key_def);
+	return 0;
+}
+
+static int
+lbox_key_def_extract_key(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL)
+		return luaL_error(L, "Usage: key_def:extract_key(tuple)");
+
+	struct tuple_format *format = box_tuple_format_default();
+	struct tuple *tuple;
+	if ((tuple = luaT_tuple_new(L, 2, format)) == NULL ||
+	    tuple_validate_parts(key_def, tuple) != 0)
+		return luaT_error(L);
+	tuple_ref(tuple);
+
+	uint32_t key_size;
+	char *key = tuple_extract_key(tuple, key_def, &key_size);
+	tuple_unref(tuple);
+	if (key == NULL)
+		return luaT_error(L);
+
+	struct tuple *ret = box_tuple_new(format, key, key + key_size);
+	if (ret == NULL)
+		return luaT_error(L);
+	luaT_pushtuple(L, ret);
+	return 1;
+}
+
+static int
+lbox_key_def_compare(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
+		return luaL_error(L, "Usage: key_def:"
+				     "compare(tuple_a, tuple_b)");
+	}
+
+	struct tuple *tuple_a, *tuple_b;
+	struct tuple_format *format = box_tuple_format_default();
+	if ((tuple_a = luaT_tuple_new(L, 2, format)) == NULL ||
+	    tuple_validate_parts(key_def, tuple_a) != 0)
+		return luaT_error(L);
+	tuple_ref(tuple_a);
+	if ((tuple_b = luaT_tuple_new(L, 3, format)) == NULL ||
+	    tuple_validate_parts(key_def, tuple_b) != 0) {
+		tuple_unref(tuple_a);
+		return luaT_error(L);
+	}
+	tuple_ref(tuple_b);
+
+	int rc = tuple_compare(tuple_a, tuple_b, key_def);
+	tuple_unref(tuple_a);
+	tuple_unref(tuple_b);
+	lua_pushinteger(L, rc);
+	return 1;
+}
+
+static int
+lbox_key_def_compare_with_key(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
+		return luaL_error(L, "Usage: key_def:"
+				     "compare_with_key(tuple, key)");
+	}
+
+	struct tuple *tuple, *key_tuple = NULL;
+	struct tuple_format *format = box_tuple_format_default();
+	if ((tuple = luaT_tuple_new(L, 2, format)) == NULL ||
+	    tuple_validate_parts(key_def, tuple) != 0)
+		return luaT_error(L);
+	tuple_ref(tuple);
+	if ((key_tuple = luaT_tuple_new(L, 3, format)) == NULL) {
+		tuple_unref(tuple);
+		return luaT_error(L);
+	}
+	tuple_ref(key_tuple);
+
+	const char *key = tuple_data(key_tuple);
+	assert(mp_typeof(*key) == MP_ARRAY);
+	uint32_t part_count = mp_decode_array(&key);
+	if (key_validate_parts(key_def, key, part_count, true) != 0) {
+		tuple_unref(tuple);
+		tuple_unref(key_tuple);
+		return luaT_error(L);
+	}
+
+	int rc = tuple_compare_with_key(tuple, key, part_count, key_def);
+	tuple_unref(tuple);
+	tuple_unref(key_tuple);
+	lua_pushinteger(L, rc);
+	return 1;
+}
+
+static int
+lbox_key_def_merge(struct 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:totable()");
+
+	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.
+ *
+ * Expected a table of key parts on the Lua stack. The format is
+ * the same as box.space.<...>.index.<...>.parts or corresponding
+ * net.box's one.
+ *
+ * Push the new key_def as cdata to a Lua stack.
+ */
+static int
+lbox_key_def_new(struct lua_State *L)
+{
+	if (lua_gettop(L) != 1 || lua_istable(L, 1) != 1)
+		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 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, "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, region) != 0) {
+			region_truncate(region, region_svp);
+			return luaT_error(L);
+		}
+	}
+
+	struct key_def *key_def = key_def_new(parts, part_count);
+	region_truncate(region, region_svp);
+	if (key_def == NULL)
+		return luaT_error(L);
+
+	/*
+	 * Calculate minimal field count of tuples with specified
+	 * key and update key_def optionality to use correct
+	 * compare/extract functions.
+	 */
+	uint32_t min_field_count =
+		tuple_format_min_field_count(&key_def, 1, NULL, 0);
+	key_def_update_optionality(key_def, min_field_count);
+
+	*(struct key_def **) luaL_pushcdata(L, key_def_type_id) = key_def;
+	lua_pushcfunction(L, lbox_key_def_gc);
+	luaL_setcdatagc(L, -2);
+
+	return 1;
+}
+
+LUA_API int
+luaopen_key_def(struct lua_State *L)
+{
+	luaL_cdef(L, "struct key_def;");
+	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
+
+	/* Export C functions to Lua. */
+	static const struct luaL_Reg meta[] = {
+		{"new", lbox_key_def_new},
+		{NULL, NULL}
+	};
+	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, "totable");
+	lua_setfield(L, -2, "internal");
+
+	return 1;
+}
diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h
new file mode 100644
index 000000000..7dc7158e8
--- /dev/null
+++ b/src/box/lua/key_def.h
@@ -0,0 +1,63 @@
+#ifndef TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
+#define TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+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.
+ */
+struct key_def *
+check_key_def(struct lua_State *L, int idx);
+
+/**
+ * Register the module.
+ */
+int
+luaopen_key_def(struct lua_State *L);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED */
diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua
new file mode 100644
index 000000000..122243cc4
--- /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,
+    ['totable'] = key_def.internal.totable,
+    ['__serialize'] = key_def.internal.totable,
+}
+
+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 ca793e423..6eecbc756 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/src/box/tuple.h b/src/box/tuple.h
index 8b12fd5a8..faa42fdf7 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -672,6 +672,39 @@ tuple_field_by_part(const struct tuple *tuple, struct key_part *part)
 				       tuple_field_map(tuple), part);
 }
 
+/**
+ * Check that tuple match with the key definition.
+ * @param key_def Key definition.
+ * @param tuple Tuple for matching.
+ * @param allow_nullable True if nullable parts are allowed.
+ *
+ * @retval 0  The tuple is valid.
+ * @retval -1 The tuple is invalid.
+ */
+static inline int
+tuple_validate_parts(struct key_def *key_def, struct tuple *tuple)
+{
+	uint32_t min_field_count =
+		tuple_format_min_field_count(&key_def, 1, NULL, 0);
+	uint32_t field_count = tuple_field_count(tuple);
+	if (field_count < min_field_count) {
+		diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1);
+		return -1;
+	}
+	for (uint32_t idx = 0; idx < key_def->part_count; idx++) {
+		struct key_part *part = &key_def->parts[idx];
+		const char *field = tuple_field_by_part(tuple, part);
+		if (field == NULL) {
+			assert(key_def->has_optional_parts);
+			continue;
+		}
+		if (key_part_validate(part->type, field, idx,
+				      key_part_is_nullable(part)) != 0)
+			return -1;
+	}
+	return 0;
+}
+
 /**
  * @brief Tuple Interator
  */
diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
new file mode 100755
index 000000000..fb2969a4d
--- /dev/null
+++ b/test/box-tap/key_def.test.lua
@@ -0,0 +1,340 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local ffi = require('ffi')
+local json = require('json')
+local fun = require('fun')
+local key_def_lib = 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>]}, ...}'
+
+local function coll_not_found(fieldno, collation)
+    if type(collation) == 'number' then
+        return ('Wrong index options (field %d): ' ..
+               'collation was not found by ID'):format(fieldno)
+    end
+
+    return ('Unknown collation: "%s"'):format(collation)
+end
+
+local function set_key_part_defaults(parts)
+    local res = {}
+    for i, part in ipairs(parts) do
+        res[i] = table.copy(part)
+        if res[i].is_nullable == nil then
+            res[i].is_nullable = false
+        end
+    end
+    return res
+end
+
+local key_def_new_cases = {
+    -- Cases to call before box.cfg{}.
+    {
+        'Pass a field on an unknown type',
+        parts = {{
+            fieldno = 2,
+            type = 'unknown',
+        }},
+        exp_err = 'Unknown field type: unknown',
+    },
+    {
+        'Try to use collation_id before box.cfg{}',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation_id = 2,
+        }},
+        exp_err = coll_not_found(1, 2),
+    },
+    {
+        'Try to use collation before box.cfg{}',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation = 'unicode_ci',
+        }},
+        exp_err = coll_not_found(1, 'unicode_ci'),
+    },
+    function()
+        -- For collations.
+        box.cfg{}
+    end,
+    -- Cases to call after box.cfg{}.
+    {
+        'Try to use both collation_id and collation',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation_id = 2,
+            collation = 'unicode_ci',
+        }},
+        exp_err = 'Conflicting options: collation_id and collation',
+    },
+    {
+        'Unknown collation_id',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation_id = 42,
+        }},
+        exp_err = coll_not_found(1, 42),
+    },
+    {
+        'Unknown collation name',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation = 'unknown',
+        }},
+        exp_err = 'Unknown collation: "unknown"',
+    },
+    {
+        'Bad parts parameter type',
+        parts = 1,
+        exp_err = usage_error,
+    },
+    {
+        'No parameters',
+        params = {},
+        exp_err = usage_error,
+    },
+    {
+        'Two parameters',
+        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 = {},
+        exp_err = nil,
+    },
+    {
+        'Success case; one part',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+        }},
+        exp_err = nil,
+    },
+    {
+        'Success case; one part with a JSON path',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            path = '[3]',
+        }},
+        exp_err = nil,
+    },
+}
+
+local test = tap.test('key_def')
+
+test:plan(#key_def_new_cases - 1 + 7)
+for _, case in ipairs(key_def_new_cases) do
+    if type(case) == 'function' then
+        case()
+    else
+        local ok, res
+        if case.params then
+            ok, res = pcall(key_def_lib.new, unpack(case.params))
+        else
+            ok, res = pcall(key_def_lib.new, case.parts)
+        end
+        if case.exp_err == nil then
+            ok = ok and type(res) == 'cdata' and
+                ffi.istype('struct key_def', res)
+            test:ok(ok, case[1])
+        else
+            local err = tostring(res) -- cdata -> string
+            test:is_deeply({ok, err}, {false, case.exp_err}, case[1])
+        end
+    end
+end
+
+-- Prepare source data for test cases.
+local parts_a = {
+    {type = 'unsigned', fieldno = 1},
+}
+local parts_b = {
+    {type = 'number', fieldno = 2},
+    {type = 'number', fieldno = 3},
+}
+local parts_c = {
+    {type = 'scalar', fieldno = 2},
+    {type = 'scalar', fieldno = 1},
+    {type = 'string', fieldno = 4, is_nullable = true},
+}
+local key_def_a = key_def_lib.new(parts_a)
+local key_def_b = key_def_lib.new(parts_b)
+local key_def_c = key_def_lib.new(parts_c)
+local tuple_a = box.tuple.new({1, 1, 22})
+local tuple_b = box.tuple.new({2, 1, 11})
+local tuple_c = box.tuple.new({3, 1, 22})
+
+-- Case: extract_key().
+test:test('extract_key()', function(test)
+    test:plan(8)
+
+    test:is_deeply(key_def_a:extract_key(tuple_a):totable(), {1}, 'case 1')
+    test:is_deeply(key_def_b:extract_key(tuple_a):totable(), {1, 22}, 'case 2')
+
+    -- JSON path.
+    local res = key_def_lib.new({
+        {type = 'string', fieldno = 1, path = 'a.b'},
+    }):extract_key(box.tuple.new({{a = {b = 'foo'}}})):totable()
+    test:is_deeply(res, {'foo'}, 'JSON path (tuple argument)')
+
+    local res = key_def_lib.new({
+        {type = 'string', fieldno = 1, path = 'a.b'},
+    }):extract_key({{a = {b = 'foo'}}}):totable()
+    test:is_deeply(res, {'foo'}, 'JSON path (table argument)')
+
+    -- A key def has a **nullable** part with a field that is over
+    -- a tuple size.
+    test:is_deeply(key_def_c:extract_key(tuple_a):totable(), {1, 1, box.NULL},
+        'short tuple with a nullable part')
+
+    -- A key def has a **non-nullable** part with a field that is
+    -- over a tuple size.
+    local exp_err = 'Field 2 was not found in the tuple'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1},
+        {type = 'string', fieldno = 2},
+    })
+    local ok, err = pcall(key_def.extract_key, key_def,
+        box.tuple.new({'foo'}))
+    test:is_deeply({ok, tostring(err)}, {false, exp_err},
+        'short tuple with a non-nullable part (case 1)')
+
+    -- Same as before, but a max fieldno is over tuple:len() + 1.
+    local exp_err = 'Field 2 was not found in the tuple'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1},
+        {type = 'string', fieldno = 2},
+        {type = 'string', fieldno = 3},
+    })
+    local ok, err = pcall(key_def.extract_key, key_def,
+        box.tuple.new({'foo'}))
+    test:is_deeply({ok, tostring(err)}, {false, exp_err},
+        'short tuple with a non-nullable part (case 2)')
+
+    local exp_err = 'Supplied key type of part 2 does not match index ' ..
+                    'part type: expected string'
+    local ok, err = pcall(key_def.extract_key, key_def,
+        {"one", "two", 3})
+    test:is_deeply({ok, tostring(err)}, {false, exp_err},
+        'short tuple with a non-nullable part (case 2)')
+end)
+
+-- Case: compare().
+test:test('compare()', function(test)
+    test:plan(8)
+
+    test:is(key_def_a:compare(tuple_b, tuple_a), 1,
+            'case 1: great (tuple argument)')
+    test:is(key_def_a:compare(tuple_b, tuple_c), -1,
+            'case 2: less (tuple argument)')
+    test:is(key_def_b:compare(tuple_b, tuple_a), -1,
+            'case 3: less (tuple argument)')
+    test:is(key_def_b:compare(tuple_a, tuple_c), 0,
+            'case 4: equal (tuple argument)')
+
+    test:is(key_def_a:compare(tuple_b:totable(), tuple_a:totable()), 1,
+            'case 1: great (table argument)')
+    test:is(key_def_a:compare(tuple_b:totable(), tuple_c:totable()), -1,
+            'case 2: less (table argument)')
+    test:is(key_def_b:compare(tuple_b:totable(), tuple_a:totable()), -1,
+            'case 3: less (table argument)')
+    test:is(key_def_b:compare(tuple_a:totable(), tuple_c:totable()), 0,
+            'case 4: equal (table argument)')
+end)
+
+-- Case: compare_with_key().
+test:test('compare_with_key()', function(test)
+    test:plan(2)
+
+    local key = {1, 22}
+    test:is(key_def_b:compare_with_key(tuple_a:totable(), key), 0, 'table')
+
+    local key = box.tuple.new({1, 22})
+    test:is(key_def_b:compare_with_key(tuple_a, key), 0, 'tuple')
+end)
+
+-- Case: totable().
+test:test('totable()', function(test)
+    test:plan(2)
+
+    local exp = set_key_part_defaults(parts_a)
+    test:is_deeply(key_def_a:totable(), exp, 'case 1')
+
+    local exp = set_key_part_defaults(parts_b)
+    test:is_deeply(key_def_b:totable(), exp, 'case 2')
+end)
+
+-- Case: __serialize().
+test:test('__serialize()', function(test)
+    test:plan(2)
+
+    local exp = set_key_part_defaults(parts_a)
+    test:is(json.encode(key_def_a), json.encode(exp), 'case 1')
+
+    local exp = set_key_part_defaults(parts_b)
+    test:is(json.encode(key_def_b), json.encode(exp), 'case 2')
+end)
+
+-- Case: tostring().
+test:test('tostring()', function(test)
+    test:plan(2)
+
+    local exp = '<struct key_def &>'
+    test:is(tostring(key_def_a), exp, 'case 1')
+    test:is(tostring(key_def_b), exp, 'case 2')
+end)
+
+-- Case: merge().
+test:test('merge()', function(test)
+    test:plan(6)
+
+    local key_def_ab = key_def_a:merge(key_def_b)
+    local exp_parts = fun.iter(key_def_a:totable())
+        :chain(fun.iter(key_def_b:totable())):totable()
+    test:is_deeply(key_def_ab:totable(), exp_parts,
+        'case 1: verify with :totable()')
+    test:is_deeply(key_def_ab:extract_key(tuple_a):totable(), {1, 1, 22},
+        'case 1: verify with :extract_key()')
+
+    local key_def_ba = key_def_b:merge(key_def_a)
+    local exp_parts = fun.iter(key_def_b:totable())
+        :chain(fun.iter(key_def_a:totable())):totable()
+    test:is_deeply(key_def_ba:totable(), exp_parts,
+        'case 2: verify with :totable()')
+    test:is_deeply(key_def_ba:extract_key(tuple_a):totable(), {1, 22, 1},
+        'case 2: verify with :extract_key()')
+
+    -- Intersecting parts + NULL parts.
+    local key_def_cb = key_def_c:merge(key_def_b)
+    local exp_parts = key_def_c:totable()
+    exp_parts[#exp_parts + 1] = {type = 'number', fieldno = 3,
+        is_nullable = false}
+    test:is_deeply(key_def_cb:totable(), exp_parts,
+        'case 3: verify with :totable()')
+    test:is_deeply(key_def_cb:extract_key(tuple_a):totable(),
+        {1, 1, box.NULL, 22}, 'case 3: verify with :extract_key()')
+end)
+
+os.exit(test:check() and 0 or 1)
-- 
2.21.0

Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

Thank you, it works like a charm.

I added a fixup commit on top of your patchset (added a test case,
updated comments in the test a bit). Also please consider comments
below.

Vladimir, I CCed you to ask a question at end of the email (the code is
on kshch/gh-4025-lua-key-kef-methods branch).

WBR, Alexander Turenko.

On Wed, Mar 27, 2019 at 05:29:28PM +0300, Kirill Shcherbatov wrote:
> There are several reasons to add this module:
> 
> * Factor out key parts parsing code from the tuples merger (#3276).
> * Support comparing a tuple with a key / a tuple, support merging
>   key_defs from Lua (#3398).
> * Support extracting a key from a tuple (#4025).
> 
> The format of `parts` parameter in the `key_def.new(parts)` call is
> compatible with the following structures:
> 
> * box.space[...].index[...].parts;
> * net_box_conn.space[...].index[...].parts.
> 
> A key_def instance has the following methods:
> 
> * :extract_key(tuple)           -> key (as tuple)
> * :compare(tuple_a, tuple_b)    -> number
> * :compare_with_key(tuple, key) -> number
> * :merge(another_key_def)       -> new key_def instance
> * :totable()                    -> table
> 

I would add here 'Functions that accept tuple(s) also allow to pass Lua
table(s) instead'.

> +static int
> +lbox_key_def_compare(struct lua_State *L)
> +{
> +	struct key_def *key_def;
> +	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
> +		return luaL_error(L, "Usage: key_def:"
> +				     "compare(tuple_a, tuple_b)");
> +	}
> +
> +	struct tuple *tuple_a, *tuple_b;
> +	struct tuple_format *format = box_tuple_format_default();
> +	if ((tuple_a = luaT_tuple_new(L, 2, format)) == NULL ||
> +	    tuple_validate_parts(key_def, tuple_a) != 0)
> +		return luaT_error(L);
> +	tuple_ref(tuple_a);
> +	if ((tuple_b = luaT_tuple_new(L, 3, format)) == NULL ||
> +	    tuple_validate_parts(key_def, tuple_b) != 0) {
> +		tuple_unref(tuple_a);
> +		return luaT_error(L);
> +	}
> +	tuple_ref(tuple_b);

Consider the case when a user get tuples from a local space (or merger)
and they have a format that allows to compare faster using precalculated
offsets. I think we should not create a new tuple(s) in the case.

Applicable for other functions too.

> diff --git a/src/box/tuple.h b/src/box/tuple.h
> index 8b12fd5a8..faa42fdf7 100644
> --- a/src/box/tuple.h
> +++ b/src/box/tuple.h
> @@ -672,6 +672,39 @@ tuple_field_by_part(const struct tuple *tuple, struct key_part *part)
>  				       tuple_field_map(tuple), part);
>  }
>  
> +/**
> + * Check that tuple match with the key definition.
> + * @param key_def Key definition.
> + * @param tuple Tuple for matching.
> + * @param allow_nullable True if nullable parts are allowed.
> + *
> + * @retval 0  The tuple is valid.
> + * @retval -1 The tuple is invalid.
> + */
> +static inline int
> +tuple_validate_parts(struct key_def *key_def, struct tuple *tuple)

I don't sure it worth to inline this function: it is not so lightweight
as, say, a structure field access.

I'm tentative whether this function should be in tuple.[ch] or
key_def.[ch]. What do you think?

(If it is in tuple.[ch] maybe it is better to let a tuple being the
first parameter?)

Maybe we need to ask Vladimir (CCed).

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Kirill Shcherbatov]

> Consider the case when a user get tuples from a local space (or merger)
> and they have a format that allows to compare faster using precalculated
> offsets. I think we should not create a new tuple(s) in the case.
> 
> Applicable for other functions too.

If we decide to support setting arguments in the form of tables, I propose to
introduce the following function. In my opinion, it does not look unnatural.

diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index 18d5d961d..9cacf3cca 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -224,6 +224,23 @@ lbox_key_def_gc(struct lua_State *L)
 	return 0;
 }
 
+/**
+ * Take existent tuple from LUA stack or build a new tuple with
+ * default format from table, check for compatibility with a
+ * given key_def.
+ */
+static struct tuple *
+lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
+{
+	struct tuple *tuple = luaT_istuple(L, idx);
+	if (tuple == NULL)
+		tuple = luaT_tuple_new(L, 2, box_tuple_format_default());
+	if (tuple == NULL || tuple_validate_parts(key_def, tuple) != 0)
+		return NULL;
+	tuple_ref(tuple);
+	return tuple;
+}
+
 static int
 lbox_key_def_extract_key(struct lua_State *L)
 {
@@ -231,12 +248,9 @@ lbox_key_def_extract_key(struct lua_State *L)
 	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL)
 		return luaL_error(L, "Usage: key_def:extract_key(tuple)");
 
-	struct tuple_format *format = box_tuple_format_default();
 	struct tuple *tuple;
-	if ((tuple = luaT_tuple_new(L, 2, format)) == NULL ||
-	    tuple_validate_parts(key_def, tuple) != 0)
+	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
 		return luaT_error(L);
-	tuple_ref(tuple);
 
 	uint32_t key_size;
 	char *key = tuple_extract_key(tuple, key_def, &key_size);
@@ -244,7 +258,8 @@ lbox_key_def_extract_key(struct lua_State *L)
 	if (key == NULL)
 		return luaT_error(L);
 
-	struct tuple *ret = box_tuple_new(format, key, key + key_size);
+	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);
@@ -261,17 +276,12 @@ lbox_key_def_compare(struct lua_State *L)
 	}
 
 	struct tuple *tuple_a, *tuple_b;
-	struct tuple_format *format = box_tuple_format_default();
-	if ((tuple_a = luaT_tuple_new(L, 2, format)) == NULL ||
-	    tuple_validate_parts(key_def, tuple_a) != 0)
+	if ((tuple_a = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
 		return luaT_error(L);
-	tuple_ref(tuple_a);
-	if ((tuple_b = luaT_tuple_new(L, 3, format)) == NULL ||
-	    tuple_validate_parts(key_def, tuple_b) != 0) {
+	if ((tuple_b = lbox_key_def_check_tuple(L, key_def, 3)) == NULL) {
 		tuple_unref(tuple_a);
 		return luaT_error(L);
 	}
-	tuple_ref(tuple_b);
 
 	int rc = tuple_compare(tuple_a, tuple_b, key_def);
 	tuple_unref(tuple_a);
@@ -291,10 +301,8 @@ lbox_key_def_compare_with_key(struct lua_State *L)
 
 	struct tuple *tuple, *key_tuple = NULL;
 	struct tuple_format *format = box_tuple_format_default();
-	if ((tuple = luaT_tuple_new(L, 2, format)) == NULL ||
-	    tuple_validate_parts(key_def, tuple) != 0)
+	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
 		return luaT_error(L);
-	tuple_ref(tuple);
 	if ((key_tuple = luaT_tuple_new(L, 3, format)) == NULL) {
 		tuple_unref(tuple);
 		return luaT_error(L);

[tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Kirill Shcherbatov]

On 28.03.2019 5:01, Alexander Turenko wrote:
> Thank you, it works like a charm.
> 
> I added a fixup commit on top of your patchset (added a test case,
> updated comments in the test a bit). Also please consider comments
> below.
Thank you!

> I would add here 'Functions that accept tuple(s) also allow to pass Lua
> table(s) instead'.
Included.

> Consider the case when a user get tuples from a local space (or merger)
> and they have a format that allows to compare faster using precalculated
> offsets. I think we should not create a new tuple(s) in the case.
> 
> Applicable for other functions too.
Introduced a new wrapper that checks existent tuple from stack or build a new
one if possible, perform validations.

> I don't sure it worth to inline this function: it is not so lightweight
> as, say, a structure field access.
> 
> I'm tentative whether this function should be in tuple.[ch] or
> key_def.[ch]. What do you think?
> 
> (If it is in tuple.[ch] maybe it is better to let a tuple being the
> first parameter?)
It must be tuple.c because we need tuple_field_by_part and some other helper
that are not defined in key_def.c

========================================================

There are several reasons to add this module:

* Factor out key parts parsing code from the tuples merger (#3276).
* Support comparing a tuple with a key / a tuple, support merging
  key_defs from Lua (#3398).
* Support extracting a key from a tuple (#4025).

The format of `parts` parameter in the `key_def.new(parts)` call is
compatible with the following structures:

* box.space[...].index[...].parts;
* net_box_conn.space[...].index[...].parts.

A key_def instance has the following methods:

* :extract_key(tuple)           -> key (as tuple)
* :compare(tuple_a, tuple_b)    -> number
* :compare_with_key(tuple, key) -> number
* :merge(another_key_def)       -> new key_def instance
* :totable()                    -> table

Note functions that accept tuple(s) also allow to pass Lua
table(s) instead.

Needed for #3276.
Fixes #3398.
Fixes #4025.

@TarantoolBot document
Title: lua: key_def module

See the commit message for an API reference.

Example for extract_key():

```lua
-- Remove values got from a secondary non-unique index.
local key_def_lib = require('key_def')
local s = box.schema.space.create('test')
local pk = s:create_index('pk')
local sk = 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}}
local key_def = key_def_lib.new(pk.parts)
for _, tuple in sk:pairs({1})) do
    local key = key_def:extract_key(tuple)
    pk:delete(key)
end
```
---
 src/CMakeLists.txt            |   1 +
 src/box/CMakeLists.txt        |   2 +
 src/box/lua/init.c            |   5 +
 src/box/lua/key_def.c         | 443 ++++++++++++++++++++++++++++++++++
 src/box/lua/key_def.h         |  63 +++++
 src/box/lua/key_def.lua       |  19 ++
 src/box/lua/space.cc          |  28 +--
 src/box/tuple.c               |  24 ++
 src/box/tuple.h               |  12 +
 test/box-tap/key_def.test.lua | 370 ++++++++++++++++++++++++++++
 10 files changed, 941 insertions(+), 26 deletions(-)
 create mode 100644 src/box/lua/key_def.c
 create mode 100644 src/box/lua/key_def.h
 create mode 100644 src/box/lua/key_def.lua
 create mode 100755 test/box-tap/key_def.test.lua

diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 7c2395517..a6a18142b 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -136,6 +136,7 @@ set(api_headers
     ${CMAKE_SOURCE_DIR}/src/lua/string.h
     ${CMAKE_SOURCE_DIR}/src/box/txn.h
     ${CMAKE_SOURCE_DIR}/src/box/key_def.h
+    ${CMAKE_SOURCE_DIR}/src/box/lua/key_def.h
     ${CMAKE_SOURCE_DIR}/src/box/field_def.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple.h
     ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h
diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
index 59e91b65a..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)
 
@@ -139,6 +140,7 @@ add_library(box STATIC
     lua/net_box.c
     lua/xlog.c
     lua/sql.c
+    lua/key_def.c
     ${bin_sources})
 
 target_link_libraries(box box_error tuple stat xrow xlog vclock crc32 scramble
diff --git a/src/box/lua/init.c b/src/box/lua/init.c
index 744b2c895..68ef58909 100644
--- a/src/box/lua/init.c
+++ b/src/box/lua/init.c
@@ -59,9 +59,11 @@
 #include "box/lua/console.h"
 #include "box/lua/tuple.h"
 #include "box/lua/sql.h"
+#include "box/lua/key_def.h"
 
 extern char session_lua[],
 	tuple_lua[],
+	key_def_lua[],
 	schema_lua[],
 	load_cfg_lua[],
 	xlog_lua[],
@@ -80,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
 };
 
@@ -312,6 +315,8 @@ box_lua_init(struct lua_State *L)
 	lua_pop(L, 1);
 	tarantool_lua_console_init(L);
 	lua_pop(L, 1);
+	luaopen_key_def(L);
+	lua_pop(L, 1);
 
 	/* Load Lua extension */
 	for (const char **s = lua_sources; *s; s += 2) {
diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
new file mode 100644
index 000000000..57c21db7b
--- /dev/null
+++ b/src/box/lua/key_def.c
@@ -0,0 +1,443 @@
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "box/coll_id_cache.h"
+#include "box/lua/key_def.h"
+#include "box/tuple.h"
+#include "diag.h"
+#include "fiber.h"
+#include "lua/utils.h"
+#include "tuple.h"
+
+static uint32_t key_def_type_id = 0;
+
+/**
+ * Set key_part_def from a table on top of a Lua stack.
+ *
+ * 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 *parts,
+		      int part_idx, struct region *region)
+{
+	struct key_part_def *part = &parts[part_idx];
+	*part = key_part_def_default;
+
+	/* Set part->fieldno. */
+	lua_pushstring(L, "fieldno");
+	lua_gettable(L, -2);
+	if (lua_isnil(L, -1)) {
+		diag_set(IllegalParams, "fieldno must not be nil");
+		return -1;
+	}
+	/*
+	 * Transform one-based Lua fieldno to zero-based
+	 * fieldno to use in key_def_new().
+	 */
+	part->fieldno = lua_tointeger(L, -1) - TUPLE_INDEX_BASE;
+	lua_pop(L, 1);
+
+	/* Set part->type. */
+	lua_pushstring(L, "type");
+	lua_gettable(L, -2);
+	if (lua_isnil(L, -1)) {
+		diag_set(IllegalParams, "type must not be nil");
+		return -1;
+	}
+	size_t type_len;
+	const char *type_name = lua_tolstring(L, -1, &type_len);
+	lua_pop(L, 1);
+	part->type = field_type_by_name(type_name, type_len);
+	switch (part->type) {
+	case FIELD_TYPE_ANY:
+	case FIELD_TYPE_ARRAY:
+	case FIELD_TYPE_MAP:
+		/* Tuple comparators don't support these types. */
+		diag_set(IllegalParams, "Unsupported field type: %s",
+			 type_name);
+		return -1;
+	case field_type_MAX:
+		diag_set(IllegalParams, "Unknown field type: %s", type_name);
+		return -1;
+	default:
+		/* Pass though. */
+		break;
+	}
+
+	/* Set part->is_nullable and part->nullable_action. */
+	lua_pushstring(L, "is_nullable");
+	lua_gettable(L, -2);
+	if (!lua_isnil(L, -1) && lua_toboolean(L, -1) != 0) {
+		part->is_nullable = true;
+		part->nullable_action = ON_CONFLICT_ACTION_NONE;
+	}
+	lua_pop(L, 1);
+
+	/*
+	 * Set part->coll_id using collation_id.
+	 *
+	 * The value will be checked in key_def_new().
+	 */
+	lua_pushstring(L, "collation_id");
+	lua_gettable(L, -2);
+	if (!lua_isnil(L, -1))
+		part->coll_id = lua_tointeger(L, -1);
+	lua_pop(L, 1);
+
+	/* Set part->coll_id using collation. */
+	lua_pushstring(L, "collation");
+	lua_gettable(L, -2);
+	if (!lua_isnil(L, -1)) {
+		/* Check for conflicting options. */
+		if (part->coll_id != COLL_NONE) {
+			diag_set(IllegalParams, "Conflicting options: "
+				 "collation_id and collation");
+			return -1;
+		}
+
+		size_t coll_name_len;
+		const char *coll_name = lua_tolstring(L, -1, &coll_name_len);
+		struct coll_id *coll_id = coll_by_name(coll_name,
+						       coll_name_len);
+		if (coll_id == NULL) {
+			diag_set(IllegalParams, "Unknown collation: \"%s\"",
+				 coll_name);
+			return -1;
+		}
+		part->coll_id = coll_id->id;
+	}
+	lua_pop(L, 1);
+
+	/* Set part->path (JSON path). */
+	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;
+		}
+		/*
+		 * lua_tolstring() guarantees that a string have
+		 * trailing '\0'.
+		 */
+		memcpy(tmp, path, path_len + 1);
+		part->path = tmp;
+	} else {
+		part->path = NULL;
+	}
+	lua_pop(L, 1);
+	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)
+{
+	if (lua_type(L, idx) != LUA_TCDATA)
+		return NULL;
+
+	uint32_t cdata_type;
+	struct key_def **key_def_ptr = luaL_checkcdata(L, idx, &cdata_type);
+	if (key_def_ptr == NULL || cdata_type != key_def_type_id)
+		return NULL;
+	return *key_def_ptr;
+}
+
+/**
+ * Free a key_def from a Lua code.
+ */
+static int
+lbox_key_def_gc(struct lua_State *L)
+{
+	struct key_def *key_def = check_key_def(L, 1);
+	if (key_def == NULL)
+		return 0;
+	box_key_def_delete(key_def);
+	return 0;
+}
+
+/**
+ * Take existent tuple from LUA stack or build a new tuple with
+ * default format from table, check for compatibility with a
+ * given key_def.
+ */
+static struct tuple *
+lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
+{
+	struct tuple *tuple = luaT_istuple(L, idx);
+	if (tuple == NULL)
+		tuple = luaT_tuple_new(L, idx, box_tuple_format_default());
+	if (tuple == NULL || box_tuple_validate_parts(tuple, key_def) != 0)
+		return NULL;
+	tuple_ref(tuple);
+	return tuple;
+}
+
+static int
+lbox_key_def_extract_key(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL)
+		return luaL_error(L, "Usage: key_def:extract_key(tuple)");
+
+	struct tuple *tuple;
+	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
+		return luaT_error(L);
+
+	uint32_t key_size;
+	char *key = tuple_extract_key(tuple, key_def, &key_size);
+	tuple_unref(tuple);
+	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(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
+		return luaL_error(L, "Usage: key_def:"
+				     "compare(tuple_a, tuple_b)");
+	}
+
+	struct tuple *tuple_a, *tuple_b;
+	if ((tuple_a = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
+		return luaT_error(L);
+	if ((tuple_b = lbox_key_def_check_tuple(L, key_def, 3)) == NULL) {
+		tuple_unref(tuple_a);
+		return luaT_error(L);
+	}
+
+	int rc = tuple_compare(tuple_a, tuple_b, key_def);
+	tuple_unref(tuple_a);
+	tuple_unref(tuple_b);
+	lua_pushinteger(L, rc);
+	return 1;
+}
+
+static int
+lbox_key_def_compare_with_key(struct lua_State *L)
+{
+	struct key_def *key_def;
+	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
+		return luaL_error(L, "Usage: key_def:"
+				     "compare_with_key(tuple, key)");
+	}
+
+	struct tuple *tuple, *key_tuple = NULL;
+	struct tuple_format *format = box_tuple_format_default();
+	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
+		return luaT_error(L);
+	if ((key_tuple = luaT_tuple_new(L, 3, format)) == NULL) {
+		tuple_unref(tuple);
+		return luaT_error(L);
+	}
+	tuple_ref(key_tuple);
+
+	const char *key = tuple_data(key_tuple);
+	assert(mp_typeof(*key) == MP_ARRAY);
+	uint32_t part_count = mp_decode_array(&key);
+	if (key_validate_parts(key_def, key, part_count, true) != 0) {
+		tuple_unref(tuple);
+		tuple_unref(key_tuple);
+		return luaT_error(L);
+	}
+
+	int rc = tuple_compare_with_key(tuple, key, part_count, key_def);
+	tuple_unref(tuple);
+	tuple_unref(key_tuple);
+	lua_pushinteger(L, rc);
+	return 1;
+}
+
+static int
+lbox_key_def_merge(struct 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:totable()");
+
+	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.
+ *
+ * Expected a table of key parts on the Lua stack. The format is
+ * the same as box.space.<...>.index.<...>.parts or corresponding
+ * net.box's one.
+ *
+ * Push the new key_def as cdata to a Lua stack.
+ */
+static int
+lbox_key_def_new(struct lua_State *L)
+{
+	if (lua_gettop(L) != 1 || lua_istable(L, 1) != 1)
+		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 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, "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, region) != 0) {
+			region_truncate(region, region_svp);
+			return luaT_error(L);
+		}
+	}
+
+	struct key_def *key_def = key_def_new(parts, part_count);
+	region_truncate(region, region_svp);
+	if (key_def == NULL)
+		return luaT_error(L);
+
+	/*
+	 * Calculate minimal field count of tuples with specified
+	 * key and update key_def optionality to use correct
+	 * compare/extract functions.
+	 */
+	uint32_t min_field_count =
+		tuple_format_min_field_count(&key_def, 1, NULL, 0);
+	key_def_update_optionality(key_def, min_field_count);
+
+	*(struct key_def **) luaL_pushcdata(L, key_def_type_id) = key_def;
+	lua_pushcfunction(L, lbox_key_def_gc);
+	luaL_setcdatagc(L, -2);
+
+	return 1;
+}
+
+LUA_API int
+luaopen_key_def(struct lua_State *L)
+{
+	luaL_cdef(L, "struct key_def;");
+	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
+
+	/* Export C functions to Lua. */
+	static const struct luaL_Reg meta[] = {
+		{"new", lbox_key_def_new},
+		{NULL, NULL}
+	};
+	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, "totable");
+	lua_setfield(L, -2, "internal");
+
+	return 1;
+}
diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h
new file mode 100644
index 000000000..7dc7158e8
--- /dev/null
+++ b/src/box/lua/key_def.h
@@ -0,0 +1,63 @@
+#ifndef TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
+#define TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
+/*
+ * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above
+ *    copyright notice, this list of conditions and the
+ *    following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above
+ *    copyright notice, this list of conditions and the following
+ *    disclaimer in the documentation and/or other materials
+ *    provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
+ * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
+ * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#if defined(__cplusplus)
+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.
+ */
+struct key_def *
+check_key_def(struct lua_State *L, int idx);
+
+/**
+ * Register the module.
+ */
+int
+luaopen_key_def(struct lua_State *L);
+
+#if defined(__cplusplus)
+} /* extern "C" */
+#endif /* defined(__cplusplus) */
+
+#endif /* TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED */
diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua
new file mode 100644
index 000000000..122243cc4
--- /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,
+    ['totable'] = key_def.internal.totable,
+    ['__serialize'] = key_def.internal.totable,
+}
+
+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 ca793e423..6eecbc756 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/src/box/tuple.c b/src/box/tuple.c
index 7f06d4053..350718eaa 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -725,6 +725,30 @@ box_tuple_new(box_tuple_format_t *format, const char *data, const char *end)
 	return tuple_bless(ret);
 }
 
+int
+box_tuple_validate_parts(const box_tuple_t *tuple, struct key_def *key_def)
+{
+	uint32_t min_field_count =
+		tuple_format_min_field_count(&key_def, 1, NULL, 0);
+	uint32_t field_count = tuple_field_count(tuple);
+	if (field_count < min_field_count) {
+		diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1);
+		return -1;
+	}
+	for (uint32_t idx = 0; idx < key_def->part_count; idx++) {
+		struct key_part *part = &key_def->parts[idx];
+		const char *field = tuple_field_by_part(tuple, part);
+		if (field == NULL) {
+			assert(key_def->has_optional_parts);
+			continue;
+		}
+		if (key_part_validate(part->type, field, idx,
+				      key_part_is_nullable(part)) != 0)
+			return -1;
+	}
+	return 0;
+}
+
 /* }}} box_tuple_* */
 
 int
diff --git a/src/box/tuple.h b/src/box/tuple.h
index 8b12fd5a8..32503b21c 100644
--- a/src/box/tuple.h
+++ b/src/box/tuple.h
@@ -284,6 +284,18 @@ box_tuple_t *
 box_tuple_upsert(const box_tuple_t *tuple, const char *expr, const
 		 char *expr_end);
 
+/**
+ * Check that tuple match with the key definition.
+ *
+ * \param key_def Key definition.
+ * \param tuple Tuple for matching.
+ * \param allow_nullable True if nullable parts are allowed.
+ * \retval 0  The tuple is valid.
+ * \retval -1 The tuple is invalid.
+ */
+int
+box_tuple_validate_parts(const box_tuple_t *tuple, struct key_def *key_def);
+
 /** \endcond public */
 
 /**
diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
new file mode 100755
index 000000000..756520b8c
--- /dev/null
+++ b/test/box-tap/key_def.test.lua
@@ -0,0 +1,370 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local ffi = require('ffi')
+local json = require('json')
+local fun = require('fun')
+local key_def_lib = 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>]}, ...}'
+
+local function coll_not_found(fieldno, collation)
+    if type(collation) == 'number' then
+        return ('Wrong index options (field %d): ' ..
+               'collation was not found by ID'):format(fieldno)
+    end
+
+    return ('Unknown collation: "%s"'):format(collation)
+end
+
+local function set_key_part_defaults(parts)
+    local res = {}
+    for i, part in ipairs(parts) do
+        res[i] = table.copy(part)
+        if res[i].is_nullable == nil then
+            res[i].is_nullable = false
+        end
+    end
+    return res
+end
+
+local key_def_new_cases = {
+    -- Cases to call before box.cfg{}.
+    {
+        'Pass a field on an unknown type',
+        parts = {{
+            fieldno = 2,
+            type = 'unknown',
+        }},
+        exp_err = 'Unknown field type: unknown',
+    },
+    {
+        'Try to use collation_id before box.cfg{}',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation_id = 2,
+        }},
+        exp_err = coll_not_found(1, 2),
+    },
+    {
+        'Try to use collation before box.cfg{}',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation = 'unicode_ci',
+        }},
+        exp_err = coll_not_found(1, 'unicode_ci'),
+    },
+    function()
+        -- For collations.
+        box.cfg{}
+    end,
+    -- Cases to call after box.cfg{}.
+    {
+        'Try to use both collation_id and collation',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation_id = 2,
+            collation = 'unicode_ci',
+        }},
+        exp_err = 'Conflicting options: collation_id and collation',
+    },
+    {
+        'Unknown collation_id',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation_id = 42,
+        }},
+        exp_err = coll_not_found(1, 42),
+    },
+    {
+        'Unknown collation name',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            collation = 'unknown',
+        }},
+        exp_err = 'Unknown collation: "unknown"',
+    },
+    {
+        'Bad parts parameter type',
+        parts = 1,
+        exp_err = usage_error,
+    },
+    {
+        'No parameters',
+        params = {},
+        exp_err = usage_error,
+    },
+    {
+        'Two parameters',
+        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 = {},
+        exp_err = nil,
+    },
+    {
+        'Success case; one part',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+        }},
+        exp_err = nil,
+    },
+    {
+        'Success case; one part with a JSON path',
+        parts = {{
+            fieldno = 1,
+            type = 'string',
+            path = '[3]',
+        }},
+        exp_err = nil,
+    },
+}
+
+local test = tap.test('key_def')
+
+test:plan(#key_def_new_cases - 1 + 7)
+for _, case in ipairs(key_def_new_cases) do
+    if type(case) == 'function' then
+        case()
+    else
+        local ok, res
+        if case.params then
+            ok, res = pcall(key_def_lib.new, unpack(case.params))
+        else
+            ok, res = pcall(key_def_lib.new, case.parts)
+        end
+        if case.exp_err == nil then
+            ok = ok and type(res) == 'cdata' and
+                ffi.istype('struct key_def', res)
+            test:ok(ok, case[1])
+        else
+            local err = tostring(res) -- cdata -> string
+            test:is_deeply({ok, err}, {false, case.exp_err}, case[1])
+        end
+    end
+end
+
+-- Prepare source data for test cases.
+local parts_a = {
+    {type = 'unsigned', fieldno = 1},
+}
+local parts_b = {
+    {type = 'number', fieldno = 2},
+    {type = 'number', fieldno = 3},
+}
+local parts_c = {
+    {type = 'scalar', fieldno = 2},
+    {type = 'scalar', fieldno = 1},
+    {type = 'string', fieldno = 4, is_nullable = true},
+}
+local key_def_a = key_def_lib.new(parts_a)
+local key_def_b = key_def_lib.new(parts_b)
+local key_def_c = key_def_lib.new(parts_c)
+local tuple_a = box.tuple.new({1, 1, 22})
+local tuple_b = box.tuple.new({2, 1, 11})
+local tuple_c = box.tuple.new({3, 1, 22})
+
+-- Case: extract_key().
+test:test('extract_key()', function(test)
+    test:plan(9)
+
+    test:is_deeply(key_def_a:extract_key(tuple_a):totable(), {1}, 'case 1')
+    test:is_deeply(key_def_b:extract_key(tuple_a):totable(), {1, 22}, 'case 2')
+
+    -- JSON path.
+    local res = key_def_lib.new({
+        {type = 'string', fieldno = 1, path = 'a.b'},
+    }):extract_key(box.tuple.new({{a = {b = 'foo'}}})):totable()
+    test:is_deeply(res, {'foo'}, 'JSON path (tuple argument)')
+
+    local res = key_def_lib.new({
+        {type = 'string', fieldno = 1, path = 'a.b'},
+    }):extract_key({{a = {b = 'foo'}}}):totable()
+    test:is_deeply(res, {'foo'}, 'JSON path (table argument)')
+
+    -- A key def has a **nullable** part with a field that is over
+    -- a tuple size.
+    --
+    -- The key def options are:
+    --
+    -- * is_nullable = true;
+    -- * has_optional_parts = true.
+    test:is_deeply(key_def_c:extract_key(tuple_a):totable(), {1, 1, box.NULL},
+        'short tuple with a nullable part')
+
+    -- A key def has a **non-nullable** part with a field that is
+    -- over a tuple size.
+    --
+    -- The key def options are:
+    --
+    -- * is_nullable = false;
+    -- * has_optional_parts = false.
+    local exp_err = 'Field 2 was not found in the tuple'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1},
+        {type = 'string', fieldno = 2},
+    })
+    local ok, err = pcall(key_def.extract_key, key_def,
+        box.tuple.new({'foo'}))
+    test:is_deeply({ok, tostring(err)}, {false, exp_err},
+        'short tuple with a non-nullable part (case 1)')
+
+    -- Same as before, but a max fieldno is over tuple:len() + 1.
+    local exp_err = 'Field 2 was not found in the tuple'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1},
+        {type = 'string', fieldno = 2},
+        {type = 'string', fieldno = 3},
+    })
+    local ok, err = pcall(key_def.extract_key, key_def,
+        box.tuple.new({'foo'}))
+    test:is_deeply({ok, tostring(err)}, {false, exp_err},
+        'short tuple with a non-nullable part (case 2)')
+
+    -- Same as before, but with another key def options:
+    --
+    -- * is_nullable = true;
+    -- * has_optional_parts = false.
+    local exp_err = 'Field 2 was not found in the tuple'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1, is_nullable = true},
+        {type = 'string', fieldno = 2},
+    })
+    local ok, err = pcall(key_def.extract_key, key_def,
+        box.tuple.new({'foo'}))
+    test:is_deeply({ok, tostring(err)}, {false, exp_err},
+        'short tuple with a non-nullable part (case 3)')
+
+    -- A tuple has a field that does not match corresponding key
+    -- part type.
+    local exp_err = 'Supplied key type of part 2 does not match index ' ..
+                    'part type: expected string'
+    local key_def = key_def_lib.new({
+        {type = 'string', fieldno = 1},
+        {type = 'string', fieldno = 2},
+        {type = 'string', fieldno = 3},
+    })
+    local ok, err = pcall(key_def.extract_key, key_def, {'one', 'two', 3})
+    test:is_deeply({ok, tostring(err)}, {false, exp_err},
+        'wrong field type')
+end)
+
+-- Case: compare().
+test:test('compare()', function(test)
+    test:plan(8)
+
+    test:is(key_def_a:compare(tuple_b, tuple_a), 1,
+            'case 1: great (tuple argument)')
+    test:is(key_def_a:compare(tuple_b, tuple_c), -1,
+            'case 2: less (tuple argument)')
+    test:is(key_def_b:compare(tuple_b, tuple_a), -1,
+            'case 3: less (tuple argument)')
+    test:is(key_def_b:compare(tuple_a, tuple_c), 0,
+            'case 4: equal (tuple argument)')
+
+    test:is(key_def_a:compare(tuple_b:totable(), tuple_a:totable()), 1,
+            'case 1: great (table argument)')
+    test:is(key_def_a:compare(tuple_b:totable(), tuple_c:totable()), -1,
+            'case 2: less (table argument)')
+    test:is(key_def_b:compare(tuple_b:totable(), tuple_a:totable()), -1,
+            'case 3: less (table argument)')
+    test:is(key_def_b:compare(tuple_a:totable(), tuple_c:totable()), 0,
+            'case 4: equal (table argument)')
+end)
+
+-- Case: compare_with_key().
+test:test('compare_with_key()', function(test)
+    test:plan(2)
+
+    local key = {1, 22}
+    test:is(key_def_b:compare_with_key(tuple_a:totable(), key), 0, 'table')
+
+    local key = box.tuple.new({1, 22})
+    test:is(key_def_b:compare_with_key(tuple_a, key), 0, 'tuple')
+end)
+
+-- Case: totable().
+test:test('totable()', function(test)
+    test:plan(2)
+
+    local exp = set_key_part_defaults(parts_a)
+    test:is_deeply(key_def_a:totable(), exp, 'case 1')
+
+    local exp = set_key_part_defaults(parts_b)
+    test:is_deeply(key_def_b:totable(), exp, 'case 2')
+end)
+
+-- Case: __serialize().
+test:test('__serialize()', function(test)
+    test:plan(2)
+
+    local exp = set_key_part_defaults(parts_a)
+    test:is(json.encode(key_def_a), json.encode(exp), 'case 1')
+
+    local exp = set_key_part_defaults(parts_b)
+    test:is(json.encode(key_def_b), json.encode(exp), 'case 2')
+end)
+
+-- Case: tostring().
+test:test('tostring()', function(test)
+    test:plan(2)
+
+    local exp = '<struct key_def &>'
+    test:is(tostring(key_def_a), exp, 'case 1')
+    test:is(tostring(key_def_b), exp, 'case 2')
+end)
+
+-- Case: merge().
+test:test('merge()', function(test)
+    test:plan(6)
+
+    local key_def_ab = key_def_a:merge(key_def_b)
+    local exp_parts = fun.iter(key_def_a:totable())
+        :chain(fun.iter(key_def_b:totable())):totable()
+    test:is_deeply(key_def_ab:totable(), exp_parts,
+        'case 1: verify with :totable()')
+    test:is_deeply(key_def_ab:extract_key(tuple_a):totable(), {1, 1, 22},
+        'case 1: verify with :extract_key()')
+
+    local key_def_ba = key_def_b:merge(key_def_a)
+    local exp_parts = fun.iter(key_def_b:totable())
+        :chain(fun.iter(key_def_a:totable())):totable()
+    test:is_deeply(key_def_ba:totable(), exp_parts,
+        'case 2: verify with :totable()')
+    test:is_deeply(key_def_ba:extract_key(tuple_a):totable(), {1, 22, 1},
+        'case 2: verify with :extract_key()')
+
+    -- Intersecting parts + NULL parts.
+    local key_def_cb = key_def_c:merge(key_def_b)
+    local exp_parts = key_def_c:totable()
+    exp_parts[#exp_parts + 1] = {type = 'number', fieldno = 3,
+        is_nullable = false}
+    test:is_deeply(key_def_cb:totable(), exp_parts,
+        'case 3: verify with :totable()')
+    test:is_deeply(key_def_cb:extract_key(tuple_a):totable(),
+        {1, 1, box.NULL, 22}, 'case 3: verify with :extract_key()')
+end)
+
+os.exit(test:check() and 0 or 1)
-- 
2.21.0

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

Changes I made (squashed and force-pushed):

* Updated a comment (see below).
* Removed #4025 from commit messages as duplicate of #3398.
* Fixed the typo: `sk:pairs({1}))` -> `sk:pairs({1})`

Vladimir, can you, please, look into the patchset?

branch: kshch/gh-4025-lua-key-kef-methods

WBR, Alexander Turenko.

On Thu, Mar 28, 2019 at 12:01:24PM +0300, Kirill Shcherbatov wrote:
> I've decided do not introduce public helper box_tuple_validate_parts, just
> inline this code.
> =================================================
> 
> There are several reasons to add this module:
> 
> * Factor out key parts parsing code from the tuples merger (#3276).
> * Support comparing a tuple with a key / a tuple, support merging
>   key_defs from Lua (#3398).
> * Support extracting a key from a tuple (#4025).
> 
> The format of `parts` parameter in the `key_def.new(parts)` call is
> compatible with the following structures:
> 
> * box.space[...].index[...].parts;
> * net_box_conn.space[...].index[...].parts.
> 
> A key_def instance has the following methods:
> 
> * :extract_key(tuple)           -> key (as tuple)
> * :compare(tuple_a, tuple_b)    -> number
> * :compare_with_key(tuple, key) -> number
> * :merge(another_key_def)       -> new key_def instance
> * :totable()                    -> table
> 
> Note functions that accept tuple(s) also allow to pass Lua
> table(s) instead.
> 
> Needed for #3276.
> Fixes #3398.
> Fixes #4025.
> 
> @TarantoolBot document
> Title: lua: key_def module
> 
> See the commit message for an API reference.
> 
> Example for extract_key():
> 
> ```lua
> -- Remove values got from a secondary non-unique index.
> local key_def_lib = require('key_def')
> local s = box.schema.space.create('test')
> local pk = s:create_index('pk')
> local sk = 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}}
> local key_def = key_def_lib.new(pk.parts)
> for _, tuple in sk:pairs({1})) do
>     local key = key_def:extract_key(tuple)
>     pk:delete(key)
> end
> ```
> ---
>  src/CMakeLists.txt            |   1 +
>  src/box/CMakeLists.txt        |   2 +
>  src/box/lua/init.c            |   5 +
>  src/box/lua/key_def.c         | 462 ++++++++++++++++++++++++++++++++++
>  src/box/lua/key_def.h         |  63 +++++
>  src/box/lua/key_def.lua       |  19 ++
>  src/box/lua/space.cc          |  28 +--
>  test/box-tap/key_def.test.lua | 370 +++++++++++++++++++++++++++
>  8 files changed, 924 insertions(+), 26 deletions(-)
>  create mode 100644 src/box/lua/key_def.c
>  create mode 100644 src/box/lua/key_def.h
>  create mode 100644 src/box/lua/key_def.lua
>  create mode 100755 test/box-tap/key_def.test.lua
> 
> diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
> index 7c2395517..a6a18142b 100644
> --- a/src/CMakeLists.txt
> +++ b/src/CMakeLists.txt
> @@ -136,6 +136,7 @@ set(api_headers
>      ${CMAKE_SOURCE_DIR}/src/lua/string.h
>      ${CMAKE_SOURCE_DIR}/src/box/txn.h
>      ${CMAKE_SOURCE_DIR}/src/box/key_def.h
> +    ${CMAKE_SOURCE_DIR}/src/box/lua/key_def.h
>      ${CMAKE_SOURCE_DIR}/src/box/field_def.h
>      ${CMAKE_SOURCE_DIR}/src/box/tuple.h
>      ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h
> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index 59e91b65a..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)
>  
> @@ -139,6 +140,7 @@ add_library(box STATIC
>      lua/net_box.c
>      lua/xlog.c
>      lua/sql.c
> +    lua/key_def.c
>      ${bin_sources})
>  
>  target_link_libraries(box box_error tuple stat xrow xlog vclock crc32 scramble
> diff --git a/src/box/lua/init.c b/src/box/lua/init.c
> index 744b2c895..68ef58909 100644
> --- a/src/box/lua/init.c
> +++ b/src/box/lua/init.c
> @@ -59,9 +59,11 @@
>  #include "box/lua/console.h"
>  #include "box/lua/tuple.h"
>  #include "box/lua/sql.h"
> +#include "box/lua/key_def.h"
>  
>  extern char session_lua[],
>  	tuple_lua[],
> +	key_def_lua[],
>  	schema_lua[],
>  	load_cfg_lua[],
>  	xlog_lua[],
> @@ -80,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
>  };
>  
> @@ -312,6 +315,8 @@ box_lua_init(struct lua_State *L)
>  	lua_pop(L, 1);
>  	tarantool_lua_console_init(L);
>  	lua_pop(L, 1);
> +	luaopen_key_def(L);
> +	lua_pop(L, 1);
>  
>  	/* Load Lua extension */
>  	for (const char **s = lua_sources; *s; s += 2) {
> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
> new file mode 100644
> index 000000000..4df6c2045
> --- /dev/null
> +++ b/src/box/lua/key_def.c
> @@ -0,0 +1,462 @@
> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#include "box/coll_id_cache.h"
> +#include "box/lua/key_def.h"
> +#include "box/tuple.h"
> +#include "diag.h"
> +#include "fiber.h"
> +#include "lua/utils.h"
> +#include "tuple.h"
> +
> +static uint32_t key_def_type_id = 0;
> +
> +/**
> + * Set key_part_def from a table on top of a Lua stack.
> + *
> + * 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 *parts,
> +		      int part_idx, struct region *region)
> +{
> +	struct key_part_def *part = &parts[part_idx];
> +	*part = key_part_def_default;
> +
> +	/* Set part->fieldno. */
> +	lua_pushstring(L, "fieldno");
> +	lua_gettable(L, -2);
> +	if (lua_isnil(L, -1)) {
> +		diag_set(IllegalParams, "fieldno must not be nil");
> +		return -1;
> +	}
> +	/*
> +	 * Transform one-based Lua fieldno to zero-based
> +	 * fieldno to use in key_def_new().
> +	 */
> +	part->fieldno = lua_tointeger(L, -1) - TUPLE_INDEX_BASE;
> +	lua_pop(L, 1);
> +
> +	/* Set part->type. */
> +	lua_pushstring(L, "type");
> +	lua_gettable(L, -2);
> +	if (lua_isnil(L, -1)) {
> +		diag_set(IllegalParams, "type must not be nil");
> +		return -1;
> +	}
> +	size_t type_len;
> +	const char *type_name = lua_tolstring(L, -1, &type_len);
> +	lua_pop(L, 1);
> +	part->type = field_type_by_name(type_name, type_len);
> +	switch (part->type) {
> +	case FIELD_TYPE_ANY:
> +	case FIELD_TYPE_ARRAY:
> +	case FIELD_TYPE_MAP:
> +		/* Tuple comparators don't support these types. */
> +		diag_set(IllegalParams, "Unsupported field type: %s",
> +			 type_name);
> +		return -1;
> +	case field_type_MAX:
> +		diag_set(IllegalParams, "Unknown field type: %s", type_name);
> +		return -1;
> +	default:
> +		/* Pass though. */
> +		break;
> +	}
> +
> +	/* Set part->is_nullable and part->nullable_action. */
> +	lua_pushstring(L, "is_nullable");
> +	lua_gettable(L, -2);
> +	if (!lua_isnil(L, -1) && lua_toboolean(L, -1) != 0) {
> +		part->is_nullable = true;
> +		part->nullable_action = ON_CONFLICT_ACTION_NONE;
> +	}
> +	lua_pop(L, 1);
> +
> +	/*
> +	 * Set part->coll_id using collation_id.
> +	 *
> +	 * The value will be checked in key_def_new().
> +	 */
> +	lua_pushstring(L, "collation_id");
> +	lua_gettable(L, -2);
> +	if (!lua_isnil(L, -1))
> +		part->coll_id = lua_tointeger(L, -1);
> +	lua_pop(L, 1);
> +
> +	/* Set part->coll_id using collation. */
> +	lua_pushstring(L, "collation");
> +	lua_gettable(L, -2);
> +	if (!lua_isnil(L, -1)) {
> +		/* Check for conflicting options. */
> +		if (part->coll_id != COLL_NONE) {
> +			diag_set(IllegalParams, "Conflicting options: "
> +				 "collation_id and collation");
> +			return -1;
> +		}
> +
> +		size_t coll_name_len;
> +		const char *coll_name = lua_tolstring(L, -1, &coll_name_len);
> +		struct coll_id *coll_id = coll_by_name(coll_name,
> +						       coll_name_len);
> +		if (coll_id == NULL) {
> +			diag_set(IllegalParams, "Unknown collation: \"%s\"",
> +				 coll_name);
> +			return -1;
> +		}
> +		part->coll_id = coll_id->id;
> +	}
> +	lua_pop(L, 1);
> +
> +	/* Set part->path (JSON path). */
> +	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;
> +		}
> +		/*
> +		 * lua_tolstring() guarantees that a string have
> +		 * trailing '\0'.
> +		 */
> +		memcpy(tmp, path, path_len + 1);
> +		part->path = tmp;
> +	} else {
> +		part->path = NULL;
> +	}
> +	lua_pop(L, 1);
> +	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)
> +{
> +	if (lua_type(L, idx) != LUA_TCDATA)
> +		return NULL;
> +
> +	uint32_t cdata_type;
> +	struct key_def **key_def_ptr = luaL_checkcdata(L, idx, &cdata_type);
> +	if (key_def_ptr == NULL || cdata_type != key_def_type_id)
> +		return NULL;
> +	return *key_def_ptr;
> +}
> +
> +/**
> + * Free a key_def from a Lua code.
> + */
> +static int
> +lbox_key_def_gc(struct lua_State *L)
> +{
> +	struct key_def *key_def = check_key_def(L, 1);
> +	if (key_def == NULL)
> +		return 0;
> +	box_key_def_delete(key_def);
> +	return 0;
> +}
> +
> +/**
> + * Take existent tuple from LUA stack or build a new tuple with
> + * default format from table, check for compatibility with a
> + * given key_def. Take tuple reference pointer on success.
> + */

Updated a bit:

diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
index 4df6c2045..dc610007b 100644
--- a/src/box/lua/key_def.c
+++ b/src/box/lua/key_def.c
@@ -219,9 +219,13 @@ lbox_key_def_gc(struct lua_State *L)
 }
 
 /**
- * Take existent tuple from LUA stack or build a new tuple with
- * default format from table, check for compatibility with a
- * given key_def. Take tuple reference pointer on success.
+ * Validate a tuple from a given index on a Lua stack against
+ * a key def and return the tuple.
+ *
+ * If a table is passed instead of a tuple, create a new tuple
+ * from it.
+ *
+ * Return a refcounted tuple (either provided one or a new one).
  */
 static struct tuple *
 lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)

> +static struct tuple *
> +lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
> +{
> +	struct tuple *tuple = luaT_istuple(L, idx);
> +	if (tuple == NULL)
> +		tuple = luaT_tuple_new(L, idx, box_tuple_format_default());
> +	if (tuple == NULL)
> +		return NULL;
> +	/* Check that tuple match with the key definition. */
> +	uint32_t min_field_count =
> +		tuple_format_min_field_count(&key_def, 1, NULL, 0);
> +	uint32_t field_count = tuple_field_count(tuple);
> +	if (field_count < min_field_count) {
> +		diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1);
> +		return NULL;
> +	}
> +	for (uint32_t idx = 0; idx < key_def->part_count; idx++) {
> +		struct key_part *part = &key_def->parts[idx];
> +		const char *field = tuple_field_by_part(tuple, part);
> +		if (field == NULL) {
> +			assert(key_def->has_optional_parts);
> +			continue;
> +		}
> +		if (key_part_validate(part->type, field, idx,
> +				      key_part_is_nullable(part)) != 0)
> +			return NULL;
> +	}
> +	tuple_ref(tuple);
> +	return tuple;
> +}
> +
> +static int
> +lbox_key_def_extract_key(struct lua_State *L)
> +{
> +	struct key_def *key_def;
> +	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL)
> +		return luaL_error(L, "Usage: key_def:extract_key(tuple)");
> +
> +	struct tuple *tuple;
> +	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
> +		return luaT_error(L);
> +
> +	uint32_t key_size;
> +	char *key = tuple_extract_key(tuple, key_def, &key_size);
> +	tuple_unref(tuple);
> +	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(struct lua_State *L)
> +{
> +	struct key_def *key_def;
> +	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
> +		return luaL_error(L, "Usage: key_def:"
> +				     "compare(tuple_a, tuple_b)");
> +	}
> +
> +	struct tuple *tuple_a, *tuple_b;
> +	if ((tuple_a = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
> +		return luaT_error(L);
> +	if ((tuple_b = lbox_key_def_check_tuple(L, key_def, 3)) == NULL) {
> +		tuple_unref(tuple_a);
> +		return luaT_error(L);
> +	}
> +
> +	int rc = tuple_compare(tuple_a, tuple_b, key_def);
> +	tuple_unref(tuple_a);
> +	tuple_unref(tuple_b);
> +	lua_pushinteger(L, rc);
> +	return 1;
> +}
> +
> +static int
> +lbox_key_def_compare_with_key(struct lua_State *L)
> +{
> +	struct key_def *key_def;
> +	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
> +		return luaL_error(L, "Usage: key_def:"
> +				     "compare_with_key(tuple, key)");
> +	}
> +
> +	struct tuple *tuple, *key_tuple = NULL;
> +	struct tuple_format *format = box_tuple_format_default();
> +	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
> +		return luaT_error(L);
> +	if ((key_tuple = luaT_tuple_new(L, 3, format)) == NULL) {
> +		tuple_unref(tuple);
> +		return luaT_error(L);
> +	}
> +	tuple_ref(key_tuple);
> +
> +	const char *key = tuple_data(key_tuple);
> +	assert(mp_typeof(*key) == MP_ARRAY);
> +	uint32_t part_count = mp_decode_array(&key);
> +	if (key_validate_parts(key_def, key, part_count, true) != 0) {
> +		tuple_unref(tuple);
> +		tuple_unref(key_tuple);
> +		return luaT_error(L);
> +	}
> +
> +	int rc = tuple_compare_with_key(tuple, key, part_count, key_def);
> +	tuple_unref(tuple);
> +	tuple_unref(key_tuple);
> +	lua_pushinteger(L, rc);
> +	return 1;
> +}
> +
> +static int
> +lbox_key_def_merge(struct 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:totable()");
> +
> +	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.
> + *
> + * Expected a table of key parts on the Lua stack. The format is
> + * the same as box.space.<...>.index.<...>.parts or corresponding
> + * net.box's one.
> + *
> + * Push the new key_def as cdata to a Lua stack.
> + */
> +static int
> +lbox_key_def_new(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1 || lua_istable(L, 1) != 1)
> +		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 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, "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, region) != 0) {
> +			region_truncate(region, region_svp);
> +			return luaT_error(L);
> +		}
> +	}
> +
> +	struct key_def *key_def = key_def_new(parts, part_count);
> +	region_truncate(region, region_svp);
> +	if (key_def == NULL)
> +		return luaT_error(L);
> +
> +	/*
> +	 * Calculate minimal field count of tuples with specified
> +	 * key and update key_def optionality to use correct
> +	 * compare/extract functions.
> +	 */
> +	uint32_t min_field_count =
> +		tuple_format_min_field_count(&key_def, 1, NULL, 0);
> +	key_def_update_optionality(key_def, min_field_count);
> +
> +	*(struct key_def **) luaL_pushcdata(L, key_def_type_id) = key_def;
> +	lua_pushcfunction(L, lbox_key_def_gc);
> +	luaL_setcdatagc(L, -2);
> +
> +	return 1;
> +}
> +
> +LUA_API int
> +luaopen_key_def(struct lua_State *L)
> +{
> +	luaL_cdef(L, "struct key_def;");
> +	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
> +
> +	/* Export C functions to Lua. */
> +	static const struct luaL_Reg meta[] = {
> +		{"new", lbox_key_def_new},
> +		{NULL, NULL}
> +	};
> +	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, "totable");
> +	lua_setfield(L, -2, "internal");
> +
> +	return 1;
> +}
> diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h
> new file mode 100644
> index 000000000..7dc7158e8
> --- /dev/null
> +++ b/src/box/lua/key_def.h
> @@ -0,0 +1,63 @@
> +#ifndef TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
> +#define TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED
> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +
> +#if defined(__cplusplus)
> +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.
> + */
> +struct key_def *
> +check_key_def(struct lua_State *L, int idx);
> +
> +/**
> + * Register the module.
> + */
> +int
> +luaopen_key_def(struct lua_State *L);
> +
> +#if defined(__cplusplus)
> +} /* extern "C" */
> +#endif /* defined(__cplusplus) */
> +
> +#endif /* TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED */
> diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua
> new file mode 100644
> index 000000000..122243cc4
> --- /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,
> +    ['totable'] = key_def.internal.totable,
> +    ['__serialize'] = key_def.internal.totable,
> +}
> +
> +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 ca793e423..6eecbc756 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
> new file mode 100755
> index 000000000..756520b8c
> --- /dev/null
> +++ b/test/box-tap/key_def.test.lua
> @@ -0,0 +1,370 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local json = require('json')
> +local fun = require('fun')
> +local key_def_lib = 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>]}, ...}'
> +
> +local function coll_not_found(fieldno, collation)
> +    if type(collation) == 'number' then
> +        return ('Wrong index options (field %d): ' ..
> +               'collation was not found by ID'):format(fieldno)
> +    end
> +
> +    return ('Unknown collation: "%s"'):format(collation)
> +end
> +
> +local function set_key_part_defaults(parts)
> +    local res = {}
> +    for i, part in ipairs(parts) do
> +        res[i] = table.copy(part)
> +        if res[i].is_nullable == nil then
> +            res[i].is_nullable = false
> +        end
> +    end
> +    return res
> +end
> +
> +local key_def_new_cases = {
> +    -- Cases to call before box.cfg{}.
> +    {
> +        'Pass a field on an unknown type',
> +        parts = {{
> +            fieldno = 2,
> +            type = 'unknown',
> +        }},
> +        exp_err = 'Unknown field type: unknown',
> +    },
> +    {
> +        'Try to use collation_id before box.cfg{}',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation_id = 2,
> +        }},
> +        exp_err = coll_not_found(1, 2),
> +    },
> +    {
> +        'Try to use collation before box.cfg{}',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation = 'unicode_ci',
> +        }},
> +        exp_err = coll_not_found(1, 'unicode_ci'),
> +    },
> +    function()
> +        -- For collations.
> +        box.cfg{}
> +    end,
> +    -- Cases to call after box.cfg{}.
> +    {
> +        'Try to use both collation_id and collation',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation_id = 2,
> +            collation = 'unicode_ci',
> +        }},
> +        exp_err = 'Conflicting options: collation_id and collation',
> +    },
> +    {
> +        'Unknown collation_id',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation_id = 42,
> +        }},
> +        exp_err = coll_not_found(1, 42),
> +    },
> +    {
> +        'Unknown collation name',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation = 'unknown',
> +        }},
> +        exp_err = 'Unknown collation: "unknown"',
> +    },
> +    {
> +        'Bad parts parameter type',
> +        parts = 1,
> +        exp_err = usage_error,
> +    },
> +    {
> +        'No parameters',
> +        params = {},
> +        exp_err = usage_error,
> +    },
> +    {
> +        'Two parameters',
> +        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 = {},
> +        exp_err = nil,
> +    },
> +    {
> +        'Success case; one part',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +        }},
> +        exp_err = nil,
> +    },
> +    {
> +        'Success case; one part with a JSON path',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            path = '[3]',
> +        }},
> +        exp_err = nil,
> +    },
> +}
> +
> +local test = tap.test('key_def')
> +
> +test:plan(#key_def_new_cases - 1 + 7)
> +for _, case in ipairs(key_def_new_cases) do
> +    if type(case) == 'function' then
> +        case()
> +    else
> +        local ok, res
> +        if case.params then
> +            ok, res = pcall(key_def_lib.new, unpack(case.params))
> +        else
> +            ok, res = pcall(key_def_lib.new, case.parts)
> +        end
> +        if case.exp_err == nil then
> +            ok = ok and type(res) == 'cdata' and
> +                ffi.istype('struct key_def', res)
> +            test:ok(ok, case[1])
> +        else
> +            local err = tostring(res) -- cdata -> string
> +            test:is_deeply({ok, err}, {false, case.exp_err}, case[1])
> +        end
> +    end
> +end
> +
> +-- Prepare source data for test cases.
> +local parts_a = {
> +    {type = 'unsigned', fieldno = 1},
> +}
> +local parts_b = {
> +    {type = 'number', fieldno = 2},
> +    {type = 'number', fieldno = 3},
> +}
> +local parts_c = {
> +    {type = 'scalar', fieldno = 2},
> +    {type = 'scalar', fieldno = 1},
> +    {type = 'string', fieldno = 4, is_nullable = true},
> +}
> +local key_def_a = key_def_lib.new(parts_a)
> +local key_def_b = key_def_lib.new(parts_b)
> +local key_def_c = key_def_lib.new(parts_c)
> +local tuple_a = box.tuple.new({1, 1, 22})
> +local tuple_b = box.tuple.new({2, 1, 11})
> +local tuple_c = box.tuple.new({3, 1, 22})
> +
> +-- Case: extract_key().
> +test:test('extract_key()', function(test)
> +    test:plan(9)
> +
> +    test:is_deeply(key_def_a:extract_key(tuple_a):totable(), {1}, 'case 1')
> +    test:is_deeply(key_def_b:extract_key(tuple_a):totable(), {1, 22}, 'case 2')
> +
> +    -- JSON path.
> +    local res = key_def_lib.new({
> +        {type = 'string', fieldno = 1, path = 'a.b'},
> +    }):extract_key(box.tuple.new({{a = {b = 'foo'}}})):totable()
> +    test:is_deeply(res, {'foo'}, 'JSON path (tuple argument)')
> +
> +    local res = key_def_lib.new({
> +        {type = 'string', fieldno = 1, path = 'a.b'},
> +    }):extract_key({{a = {b = 'foo'}}}):totable()
> +    test:is_deeply(res, {'foo'}, 'JSON path (table argument)')
> +
> +    -- A key def has a **nullable** part with a field that is over
> +    -- a tuple size.
> +    --
> +    -- The key def options are:
> +    --
> +    -- * is_nullable = true;
> +    -- * has_optional_parts = true.
> +    test:is_deeply(key_def_c:extract_key(tuple_a):totable(), {1, 1, box.NULL},
> +        'short tuple with a nullable part')
> +
> +    -- A key def has a **non-nullable** part with a field that is
> +    -- over a tuple size.
> +    --
> +    -- The key def options are:
> +    --
> +    -- * is_nullable = false;
> +    -- * has_optional_parts = false.
> +    local exp_err = 'Field 2 was not found in the tuple'
> +    local key_def = key_def_lib.new({
> +        {type = 'string', fieldno = 1},
> +        {type = 'string', fieldno = 2},
> +    })
> +    local ok, err = pcall(key_def.extract_key, key_def,
> +        box.tuple.new({'foo'}))
> +    test:is_deeply({ok, tostring(err)}, {false, exp_err},
> +        'short tuple with a non-nullable part (case 1)')
> +
> +    -- Same as before, but a max fieldno is over tuple:len() + 1.
> +    local exp_err = 'Field 2 was not found in the tuple'
> +    local key_def = key_def_lib.new({
> +        {type = 'string', fieldno = 1},
> +        {type = 'string', fieldno = 2},
> +        {type = 'string', fieldno = 3},
> +    })
> +    local ok, err = pcall(key_def.extract_key, key_def,
> +        box.tuple.new({'foo'}))
> +    test:is_deeply({ok, tostring(err)}, {false, exp_err},
> +        'short tuple with a non-nullable part (case 2)')
> +
> +    -- Same as before, but with another key def options:
> +    --
> +    -- * is_nullable = true;
> +    -- * has_optional_parts = false.
> +    local exp_err = 'Field 2 was not found in the tuple'
> +    local key_def = key_def_lib.new({
> +        {type = 'string', fieldno = 1, is_nullable = true},
> +        {type = 'string', fieldno = 2},
> +    })
> +    local ok, err = pcall(key_def.extract_key, key_def,
> +        box.tuple.new({'foo'}))
> +    test:is_deeply({ok, tostring(err)}, {false, exp_err},
> +        'short tuple with a non-nullable part (case 3)')
> +
> +    -- A tuple has a field that does not match corresponding key
> +    -- part type.
> +    local exp_err = 'Supplied key type of part 2 does not match index ' ..
> +                    'part type: expected string'
> +    local key_def = key_def_lib.new({
> +        {type = 'string', fieldno = 1},
> +        {type = 'string', fieldno = 2},
> +        {type = 'string', fieldno = 3},
> +    })
> +    local ok, err = pcall(key_def.extract_key, key_def, {'one', 'two', 3})
> +    test:is_deeply({ok, tostring(err)}, {false, exp_err},
> +        'wrong field type')
> +end)
> +
> +-- Case: compare().
> +test:test('compare()', function(test)
> +    test:plan(8)
> +
> +    test:is(key_def_a:compare(tuple_b, tuple_a), 1,
> +            'case 1: great (tuple argument)')
> +    test:is(key_def_a:compare(tuple_b, tuple_c), -1,
> +            'case 2: less (tuple argument)')
> +    test:is(key_def_b:compare(tuple_b, tuple_a), -1,
> +            'case 3: less (tuple argument)')
> +    test:is(key_def_b:compare(tuple_a, tuple_c), 0,
> +            'case 4: equal (tuple argument)')
> +
> +    test:is(key_def_a:compare(tuple_b:totable(), tuple_a:totable()), 1,
> +            'case 1: great (table argument)')
> +    test:is(key_def_a:compare(tuple_b:totable(), tuple_c:totable()), -1,
> +            'case 2: less (table argument)')
> +    test:is(key_def_b:compare(tuple_b:totable(), tuple_a:totable()), -1,
> +            'case 3: less (table argument)')
> +    test:is(key_def_b:compare(tuple_a:totable(), tuple_c:totable()), 0,
> +            'case 4: equal (table argument)')
> +end)
> +
> +-- Case: compare_with_key().
> +test:test('compare_with_key()', function(test)
> +    test:plan(2)
> +
> +    local key = {1, 22}
> +    test:is(key_def_b:compare_with_key(tuple_a:totable(), key), 0, 'table')
> +
> +    local key = box.tuple.new({1, 22})
> +    test:is(key_def_b:compare_with_key(tuple_a, key), 0, 'tuple')
> +end)
> +
> +-- Case: totable().
> +test:test('totable()', function(test)
> +    test:plan(2)
> +
> +    local exp = set_key_part_defaults(parts_a)
> +    test:is_deeply(key_def_a:totable(), exp, 'case 1')
> +
> +    local exp = set_key_part_defaults(parts_b)
> +    test:is_deeply(key_def_b:totable(), exp, 'case 2')
> +end)
> +
> +-- Case: __serialize().
> +test:test('__serialize()', function(test)
> +    test:plan(2)
> +
> +    local exp = set_key_part_defaults(parts_a)
> +    test:is(json.encode(key_def_a), json.encode(exp), 'case 1')
> +
> +    local exp = set_key_part_defaults(parts_b)
> +    test:is(json.encode(key_def_b), json.encode(exp), 'case 2')
> +end)
> +
> +-- Case: tostring().
> +test:test('tostring()', function(test)
> +    test:plan(2)
> +
> +    local exp = '<struct key_def &>'
> +    test:is(tostring(key_def_a), exp, 'case 1')
> +    test:is(tostring(key_def_b), exp, 'case 2')
> +end)
> +
> +-- Case: merge().
> +test:test('merge()', function(test)
> +    test:plan(6)
> +
> +    local key_def_ab = key_def_a:merge(key_def_b)
> +    local exp_parts = fun.iter(key_def_a:totable())
> +        :chain(fun.iter(key_def_b:totable())):totable()
> +    test:is_deeply(key_def_ab:totable(), exp_parts,
> +        'case 1: verify with :totable()')
> +    test:is_deeply(key_def_ab:extract_key(tuple_a):totable(), {1, 1, 22},
> +        'case 1: verify with :extract_key()')
> +
> +    local key_def_ba = key_def_b:merge(key_def_a)
> +    local exp_parts = fun.iter(key_def_b:totable())
> +        :chain(fun.iter(key_def_a:totable())):totable()
> +    test:is_deeply(key_def_ba:totable(), exp_parts,
> +        'case 2: verify with :totable()')
> +    test:is_deeply(key_def_ba:extract_key(tuple_a):totable(), {1, 22, 1},
> +        'case 2: verify with :extract_key()')
> +
> +    -- Intersecting parts + NULL parts.
> +    local key_def_cb = key_def_c:merge(key_def_b)
> +    local exp_parts = key_def_c:totable()
> +    exp_parts[#exp_parts + 1] = {type = 'number', fieldno = 3,
> +        is_nullable = false}
> +    test:is_deeply(key_def_cb:totable(), exp_parts,
> +        'case 3: verify with :totable()')
> +    test:is_deeply(key_def_cb:extract_key(tuple_a):totable(),
> +        {1, 1, box.NULL, 22}, 'case 3: verify with :extract_key()')
> +end)
> +
> +os.exit(test:check() and 0 or 1)
> -- 
> 2.21.0
> 

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Vladimir Davydov]

On Thu, Mar 28, 2019 at 02:22:01PM +0300, Alexander Turenko wrote:
> Changes I made (squashed and force-pushed):
> 
> * Updated a comment (see below).
> * Removed #4025 from commit messages as duplicate of #3398.
> * Fixed the typo: `sk:pairs({1}))` -> `sk:pairs({1})`
> 
> Vladimir, can you, please, look into the patchset?

Patch 1 looks good to me.

Pasting patch 2 here for review:

> From 0ba422c6f9a4daa4716275ebb256a9e76e49aea1 Mon Sep 17 00:00:00 2001
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> Date: Mon, 7 Jan 2019 19:12:50 +0300
> Subject: [PATCH] lua: add key_def lua module
> 
> There are several reasons to add this module:
> 
> * Factor out key parts parsing code from the tuples merger (#3276).
> * Support comparing a tuple with a key / a tuple, support merging
>   key_defs from Lua (#3398).
> * Support extracting a key from a tuple (#4025).
> 
> The format of `parts` parameter in the `key_def.new(parts)` call is
> compatible with the following structures:
> 
> * box.space[...].index[...].parts;
> * net_box_conn.space[...].index[...].parts.
> 
> A key_def instance has the following methods:
> 
> * :extract_key(tuple)           -> key (as tuple)
> * :compare(tuple_a, tuple_b)    -> number
> * :compare_with_key(tuple, key) -> number

What number do these functions return?

> * :merge(another_key_def)       -> new key_def instance

What does 'merge' do?

> * :totable()                    -> table

Does this function return key_def parts? In what format?
Please elaborate the comments.

Also, key_def.compare() sounds like it compares key definitions, not
tuples. May be, we should move these functions to box.tuple module?

Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better
introduce 'equal', 'greater', 'less', etc helpers returning bool?

I'm not strongly against the proposed API, but I think we should agree
on it with other members of the team, potential users, and Kostja.

Also, how is this module supposed to handle multikey indexes?

> 
> Note functions that accept tuple(s) also allow to pass Lua
> table(s) instead.
> 
> Needed for #3276.
> Fixes #3398.
> Fixes #4025.
> 
> @TarantoolBot document
> Title: lua: key_def module
> 
> See the commit message for an API reference.

Rather than referring to the commit message, please move @TarantoolBot
request above.

> 
> Example for extract_key():
> 
> ```lua
> -- Remove values got from a secondary non-unique index.
> local key_def_lib = require('key_def')
> local s = box.schema.space.create('test')
> local pk = s:create_index('pk')
> local sk = 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}}
> local key_def = key_def_lib.new(pk.parts)
> for _, tuple in sk:pairs({1})) do
>     local key = key_def:extract_key(tuple)
>     pk:delete(key)
> end
> ```
> 
> diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c
> new file mode 100644
> index 00000000..4df6c204
> --- /dev/null
> +++ b/src/box/lua/key_def.c
> +/**
> + * Set key_part_def from a table on top of a Lua stack.
> + *
> + * 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 *parts,
> +		      int part_idx, struct region *region)

What is the region needed for? Please mention in the comment.

> +{
> +	struct key_part_def *part = &parts[part_idx];
> +	*part = key_part_def_default;
> +
> +	/* Set part->fieldno. */
> +	lua_pushstring(L, "fieldno");
> +	lua_gettable(L, -2);
> +	if (lua_isnil(L, -1)) {
> +		diag_set(IllegalParams, "fieldno must not be nil");
> +		return -1;
> +	}
> +	/*
> +	 * Transform one-based Lua fieldno to zero-based
> +	 * fieldno to use in key_def_new().
> +	 */
> +	part->fieldno = lua_tointeger(L, -1) - TUPLE_INDEX_BASE;
> +	lua_pop(L, 1);
> +
> +	/* Set part->type. */
> +	lua_pushstring(L, "type");
> +	lua_gettable(L, -2);
> +	if (lua_isnil(L, -1)) {
> +		diag_set(IllegalParams, "type must not be nil");
> +		return -1;
> +	}
> +	size_t type_len;
> +	const char *type_name = lua_tolstring(L, -1, &type_len);
> +	lua_pop(L, 1);
> +	part->type = field_type_by_name(type_name, type_len);
> +	switch (part->type) {
> +	case FIELD_TYPE_ANY:
> +	case FIELD_TYPE_ARRAY:
> +	case FIELD_TYPE_MAP:
> +		/* Tuple comparators don't support these types. */
> +		diag_set(IllegalParams, "Unsupported field type: %s",
> +			 type_name);
> +		return -1;
> +	case field_type_MAX:
> +		diag_set(IllegalParams, "Unknown field type: %s", type_name);
> +		return -1;
> +	default:
> +		/* Pass though. */
> +		break;
> +	}
> +
> +	/* Set part->is_nullable and part->nullable_action. */
> +	lua_pushstring(L, "is_nullable");
> +	lua_gettable(L, -2);
> +	if (!lua_isnil(L, -1) && lua_toboolean(L, -1) != 0) {
> +		part->is_nullable = true;
> +		part->nullable_action = ON_CONFLICT_ACTION_NONE;
> +	}
> +	lua_pop(L, 1);
> +
> +	/*
> +	 * Set part->coll_id using collation_id.
> +	 *
> +	 * The value will be checked in key_def_new().
> +	 */
> +	lua_pushstring(L, "collation_id");
> +	lua_gettable(L, -2);
> +	if (!lua_isnil(L, -1))
> +		part->coll_id = lua_tointeger(L, -1);
> +	lua_pop(L, 1);
> +
> +	/* Set part->coll_id using collation. */
> +	lua_pushstring(L, "collation");
> +	lua_gettable(L, -2);
> +	if (!lua_isnil(L, -1)) {
> +		/* Check for conflicting options. */
> +		if (part->coll_id != COLL_NONE) {
> +			diag_set(IllegalParams, "Conflicting options: "
> +				 "collation_id and collation");
> +			return -1;
> +		}
> +
> +		size_t coll_name_len;
> +		const char *coll_name = lua_tolstring(L, -1, &coll_name_len);
> +		struct coll_id *coll_id = coll_by_name(coll_name,
> +						       coll_name_len);
> +		if (coll_id == NULL) {
> +			diag_set(IllegalParams, "Unknown collation: \"%s\"",
> +				 coll_name);
> +			return -1;
> +		}
> +		part->coll_id = coll_id->id;
> +	}
> +	lua_pop(L, 1);
> +
> +	/* Set part->path (JSON path). */
> +	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");

Mixing ER_WRONG_INDEX_OPTIONS and IllegalParams looks ugly. Please fix.

> +			return -1;
> +		}
> +		char *tmp = region_alloc(region, path_len + 1);
> +		if (tmp == NULL) {
> +			diag_set(OutOfMemory, path_len + 1, "region", "path");
> +			return -1;
> +		}
> +		/*
> +		 * lua_tolstring() guarantees that a string have
> +		 * trailing '\0'.
> +		 */
> +		memcpy(tmp, path, path_len + 1);
> +		part->path = tmp;
> +	} else {
> +		part->path = NULL;
> +	}
> +	lua_pop(L, 1);
> +	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)

Please prefix the name with lbox_ or... I dunno - the naming looks
inconsistent: luaT_key_def_set_part, lbox_push_key_part, check_key_def.
Is there some kind of pattern?

> +{
> +	if (lua_type(L, idx) != LUA_TCDATA)
> +		return NULL;
> +
> +	uint32_t cdata_type;
> +	struct key_def **key_def_ptr = luaL_checkcdata(L, idx, &cdata_type);
> +	if (key_def_ptr == NULL || cdata_type != key_def_type_id)
> +		return NULL;
> +	return *key_def_ptr;
> +}
> +
> +/**
> + * Free a key_def from a Lua code.
> + */
> +static int
> +lbox_key_def_gc(struct lua_State *L)
> +{
> +	struct key_def *key_def = check_key_def(L, 1);
> +	if (key_def == NULL)
> +		return 0;
> +	box_key_def_delete(key_def);
> +	return 0;
> +}
> +
> +/**
> + * Take existent tuple from LUA stack or build a new tuple with
> + * default format from table, check for compatibility with a
> + * given key_def. Take tuple reference pointer on success.
> + */
> +static struct tuple *
> +lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
> +{
> +	struct tuple *tuple = luaT_istuple(L, idx);
> +	if (tuple == NULL)
> +		tuple = luaT_tuple_new(L, idx, box_tuple_format_default());
> +	if (tuple == NULL)
> +		return NULL;
> +	/* Check that tuple match with the key definition. */
> +	uint32_t min_field_count =
> +		tuple_format_min_field_count(&key_def, 1, NULL, 0);
> +	uint32_t field_count = tuple_field_count(tuple);
> +	if (field_count < min_field_count) {
> +		diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1);
> +		return NULL;
> +	}
> +	for (uint32_t idx = 0; idx < key_def->part_count; idx++) {
> +		struct key_part *part = &key_def->parts[idx];
> +		const char *field = tuple_field_by_part(tuple, part);
> +		if (field == NULL) {
> +			assert(key_def->has_optional_parts);
> +			continue;
> +		}
> +		if (key_part_validate(part->type, field, idx,
> +				      key_part_is_nullable(part)) != 0)
> +			return NULL;
> +	}
> +	tuple_ref(tuple);
> +	return tuple;
> +}

The code checking a tuple against key_def should live somewhere in
src/box - chances are high that we miss lua/key_def.c when we extend
key_def struct again.

Anyway, this check is incorrect:

	tarantool> s = box.schema.space.create('test')
	---
	...

	tarantool> i = box.space.test:create_index('pk', {parts = {{1, 'unsigned', path = 'a'}, {1, 'unsigned', path = 'b'} }})
	---
	...

	tarantool> k = require('key_def').new(i.parts)
	---
	...

	tarantool> k:extract_key({1, 2, 3})
	tarantool: /home/vlad/src/tarantool/src/box/lua/key_def.c:246: lbox_key_def_check_tuple: Assertion `key_def->has_optional_parts' failed.

Please fix it and don't forget to add a test case.

Probably, we should reuse tuple_validate() for checking a tuple against
a key_def so as not to implement the same code again.

> +
> +static int
> +lbox_key_def_extract_key(struct lua_State *L)
> +{
> +	struct key_def *key_def;
> +	if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL)
> +		return luaL_error(L, "Usage: key_def:extract_key(tuple)");
> +
> +	struct tuple *tuple;
> +	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
> +		return luaT_error(L);
> +
> +	uint32_t key_size;
> +	char *key = tuple_extract_key(tuple, key_def, &key_size);
> +	tuple_unref(tuple);
> +	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(struct lua_State *L)
> +{
> +	struct key_def *key_def;
> +	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
> +		return luaL_error(L, "Usage: key_def:"
> +				     "compare(tuple_a, tuple_b)");
> +	}
> +
> +	struct tuple *tuple_a, *tuple_b;
> +	if ((tuple_a = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
> +		return luaT_error(L);
> +	if ((tuple_b = lbox_key_def_check_tuple(L, key_def, 3)) == NULL) {
> +		tuple_unref(tuple_a);
> +		return luaT_error(L);
> +	}
> +
> +	int rc = tuple_compare(tuple_a, tuple_b, key_def);
> +	tuple_unref(tuple_a);
> +	tuple_unref(tuple_b);
> +	lua_pushinteger(L, rc);
> +	return 1;
> +}
> +
> +static int
> +lbox_key_def_compare_with_key(struct lua_State *L)
> +{
> +	struct key_def *key_def;
> +	if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL) {
> +		return luaL_error(L, "Usage: key_def:"
> +				     "compare_with_key(tuple, key)");
> +	}
> +
> +	struct tuple *tuple, *key_tuple = NULL;
> +	struct tuple_format *format = box_tuple_format_default();
> +	if ((tuple = lbox_key_def_check_tuple(L, key_def, 2)) == NULL)
> +		return luaT_error(L);
> +	if ((key_tuple = luaT_tuple_new(L, 3, format)) == NULL) {
> +		tuple_unref(tuple);
> +		return luaT_error(L);
> +	}
> +	tuple_ref(key_tuple);
> +
> +	const char *key = tuple_data(key_tuple);

Creating a tuple even though we just need msgpack doesn't look good to
me. Can we encode msgpack without creating a tuple? How does index.get
handle it?

> +	assert(mp_typeof(*key) == MP_ARRAY);
> +	uint32_t part_count = mp_decode_array(&key);
> +	if (key_validate_parts(key_def, key, part_count, true) != 0) {
> +		tuple_unref(tuple);
> +		tuple_unref(key_tuple);
> +		return luaT_error(L);
> +	}
> +
> +	int rc = tuple_compare_with_key(tuple, key, part_count, key_def);
> +	tuple_unref(tuple);
> +	tuple_unref(key_tuple);
> +	lua_pushinteger(L, rc);
> +	return 1;
> +}
> +
> +static int
> +lbox_key_def_merge(struct 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:totable()");
> +
> +	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.
> + *
> + * Expected a table of key parts on the Lua stack. The format is
> + * the same as box.space.<...>.index.<...>.parts or corresponding
> + * net.box's one.
> + *
> + * Push the new key_def as cdata to a Lua stack.
> + */
> +static int
> +lbox_key_def_new(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1 || lua_istable(L, 1) != 1)
> +		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 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, "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, region) != 0) {
> +			region_truncate(region, region_svp);
> +			return luaT_error(L);
> +		}
> +	}
> +
> +	struct key_def *key_def = key_def_new(parts, part_count);
> +	region_truncate(region, region_svp);
> +	if (key_def == NULL)
> +		return luaT_error(L);
> +
> +	/*
> +	 * Calculate minimal field count of tuples with specified
> +	 * key and update key_def optionality to use correct
> +	 * compare/extract functions.
> +	 */
> +	uint32_t min_field_count =
> +		tuple_format_min_field_count(&key_def, 1, NULL, 0);
> +	key_def_update_optionality(key_def, min_field_count);
> +
> +	*(struct key_def **) luaL_pushcdata(L, key_def_type_id) = key_def;
> +	lua_pushcfunction(L, lbox_key_def_gc);
> +	luaL_setcdatagc(L, -2);
> +
> +	return 1;
> +}
> +
> +LUA_API int
> +luaopen_key_def(struct lua_State *L)
> +{
> +	luaL_cdef(L, "struct key_def;");
> +	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
> +
> +	/* Export C functions to Lua. */
> +	static const struct luaL_Reg meta[] = {
> +		{"new", lbox_key_def_new},
> +		{NULL, NULL}
> +	};
> +	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, "totable");
> +	lua_setfield(L, -2, "internal");

Why 'internal'? We use them as is as key_def methods.
E.g. box.tuple.* methods aren't internal.

> +
> +	return 1;
> +}
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index ca793e42..6eecbc75 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] */

Why not factor out lbox_push_key_def instead?

>  		}
>  
> diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua
> new file mode 100755
> index 00000000..1270f82e
> --- /dev/null
> +++ b/test/box-tap/key_def.test.lua
> @@ -0,0 +1,370 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local ffi = require('ffi')
> +local json = require('json')
> +local fun = require('fun')
> +local key_def_lib = 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>]}, ...}'
> +
> +local function coll_not_found(fieldno, collation)
> +    if type(collation) == 'number' then
> +        return ('Wrong index options (field %d): ' ..
> +               'collation was not found by ID'):format(fieldno)
> +    end
> +
> +    return ('Unknown collation: "%s"'):format(collation)
> +end
> +
> +local function set_key_part_defaults(parts)
> +    local res = {}
> +    for i, part in ipairs(parts) do
> +        res[i] = table.copy(part)
> +        if res[i].is_nullable == nil then
> +            res[i].is_nullable = false
> +        end
> +    end
> +    return res
> +end
> +
> +local key_def_new_cases = {
> +    -- Cases to call before box.cfg{}.
> +    {
> +        'Pass a field on an unknown type',
> +        parts = {{
> +            fieldno = 2,
> +            type = 'unknown',
> +        }},
> +        exp_err = 'Unknown field type: unknown',
> +    },
> +    {
> +        'Try to use collation_id before box.cfg{}',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation_id = 2,
> +        }},
> +        exp_err = coll_not_found(1, 2),
> +    },
> +    {
> +        'Try to use collation before box.cfg{}',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation = 'unicode_ci',
> +        }},
> +        exp_err = coll_not_found(1, 'unicode_ci'),
> +    },
> +    function()
> +        -- For collations.
> +        box.cfg{}
> +    end,
> +    -- Cases to call after box.cfg{}.
> +    {
> +        'Try to use both collation_id and collation',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation_id = 2,
> +            collation = 'unicode_ci',
> +        }},
> +        exp_err = 'Conflicting options: collation_id and collation',
> +    },
> +    {
> +        'Unknown collation_id',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation_id = 999,
> +        }},
> +        exp_err = coll_not_found(1, 999),
> +    },
> +    {
> +        'Unknown collation name',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            collation = 'unknown',
> +        }},
> +        exp_err = 'Unknown collation: "unknown"',
> +    },
> +    {
> +        'Bad parts parameter type',
> +        parts = 1,
> +        exp_err = usage_error,
> +    },
> +    {
> +        'No parameters',
> +        params = {},
> +        exp_err = usage_error,
> +    },
> +    {
> +        'Two parameters',
> +        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 = {},
> +        exp_err = nil,
> +    },
> +    {
> +        'Success case; one part',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +        }},
> +        exp_err = nil,
> +    },
> +    {
> +        'Success case; one part with a JSON path',
> +        parts = {{
> +            fieldno = 1,
> +            type = 'string',
> +            path = '[3]',
> +        }},
> +        exp_err = nil,
> +    },
> +}
> +
> +local test = tap.test('key_def')
> +
> +test:plan(#key_def_new_cases - 1 + 7)
> +for _, case in ipairs(key_def_new_cases) do
> +    if type(case) == 'function' then
> +        case()
> +    else
> +        local ok, res
> +        if case.params then
> +            ok, res = pcall(key_def_lib.new, unpack(case.params))
> +        else
> +            ok, res = pcall(key_def_lib.new, case.parts)
> +        end
> +        if case.exp_err == nil then
> +            ok = ok and type(res) == 'cdata' and
> +                ffi.istype('struct key_def', res)
> +            test:ok(ok, case[1])
> +        else
> +            local err = tostring(res) -- cdata -> string
> +            test:is_deeply({ok, err}, {false, case.exp_err}, case[1])
> +        end
> +    end
> +end
> +
> +-- Prepare source data for test cases.
> +local parts_a = {
> +    {type = 'unsigned', fieldno = 1},
> +}
> +local parts_b = {
> +    {type = 'number', fieldno = 2},
> +    {type = 'number', fieldno = 3},
> +}
> +local parts_c = {
> +    {type = 'scalar', fieldno = 2},
> +    {type = 'scalar', fieldno = 1},
> +    {type = 'string', fieldno = 4, is_nullable = true},
> +}
> +local key_def_a = key_def_lib.new(parts_a)
> +local key_def_b = key_def_lib.new(parts_b)
> +local key_def_c = key_def_lib.new(parts_c)
> +local tuple_a = box.tuple.new({1, 1, 22})
> +local tuple_b = box.tuple.new({2, 1, 11})
> +local tuple_c = box.tuple.new({3, 1, 22})
> +
> +-- Case: extract_key().
> +test:test('extract_key()', function(test)
> +    test:plan(9)
> +
> +    test:is_deeply(key_def_a:extract_key(tuple_a):totable(), {1}, 'case 1')
> +    test:is_deeply(key_def_b:extract_key(tuple_a):totable(), {1, 22}, 'case 2')
> +
> +    -- JSON path.
> +    local res = key_def_lib.new({
> +        {type = 'string', fieldno = 1, path = 'a.b'},
> +    }):extract_key(box.tuple.new({{a = {b = 'foo'}}})):totable()
> +    test:is_deeply(res, {'foo'}, 'JSON path (tuple argument)')
> +
> +    local res = key_def_lib.new({
> +        {type = 'string', fieldno = 1, path = 'a.b'},
> +    }):extract_key({{a = {b = 'foo'}}}):totable()
> +    test:is_deeply(res, {'foo'}, 'JSON path (table argument)')

I like key_def_new_cases - they are very easy to read or extend.
I don't quite like the tests below, because they refer to objects
created a few screens above (tuple_a, key_def_a, etc). Could you
please rewrite them in a similar to key_def_new_cases fashion,
without referring to any predefined variables?

Also, it looks like you don't test json tuple comparison properly.
Please add more test cases.

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

> > +-- Case: extract_key().
> > +test:test('extract_key()', function(test)
> > +    test:plan(9)
> > +
> > +    test:is_deeply(key_def_a:extract_key(tuple_a):totable(), {1}, 'case 1')
> > +    test:is_deeply(key_def_b:extract_key(tuple_a):totable(), {1, 22}, 'case 2')
> > +
> > +    -- JSON path.
> > +    local res = key_def_lib.new({
> > +        {type = 'string', fieldno = 1, path = 'a.b'},
> > +    }):extract_key(box.tuple.new({{a = {b = 'foo'}}})):totable()
> > +    test:is_deeply(res, {'foo'}, 'JSON path (tuple argument)')
> > +
> > +    local res = key_def_lib.new({
> > +        {type = 'string', fieldno = 1, path = 'a.b'},
> > +    }):extract_key({{a = {b = 'foo'}}}):totable()
> > +    test:is_deeply(res, {'foo'}, 'JSON path (table argument)')
> 
> I like key_def_new_cases - they are very easy to read or extend.
> I don't quite like the tests below, because they refer to objects
> created a few screens above (tuple_a, key_def_a, etc). Could you
> please rewrite them in a similar to key_def_new_cases fashion,
> without referring to any predefined variables?

It is easy to separate test cases from a testing code in case of one
function like key_def.new(), but it is not so easy when we need to test
several functions with different behaviour. So I vote up for inlining
related data (tuple_a and so on) to test cases, but doubt these cases
could be written in such declarative manner as I did for key_def.new().

Thank you for the detailed review (eps. for the case that passes over
validation)!

WBR, Alexander Turenko.

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Vladimir Davydov]

On Wed, Apr 03, 2019 at 02:46:44PM +0300, Alexander Turenko wrote:
> > > +-- Case: extract_key().
> > > +test:test('extract_key()', function(test)
> > > +    test:plan(9)
> > > +
> > > +    test:is_deeply(key_def_a:extract_key(tuple_a):totable(), {1}, 'case 1')
> > > +    test:is_deeply(key_def_b:extract_key(tuple_a):totable(), {1, 22}, 'case 2')
> > > +
> > > +    -- JSON path.
> > > +    local res = key_def_lib.new({
> > > +        {type = 'string', fieldno = 1, path = 'a.b'},
> > > +    }):extract_key(box.tuple.new({{a = {b = 'foo'}}})):totable()
> > > +    test:is_deeply(res, {'foo'}, 'JSON path (tuple argument)')
> > > +
> > > +    local res = key_def_lib.new({
> > > +        {type = 'string', fieldno = 1, path = 'a.b'},
> > > +    }):extract_key({{a = {b = 'foo'}}}):totable()
> > > +    test:is_deeply(res, {'foo'}, 'JSON path (table argument)')
> > 
> > I like key_def_new_cases - they are very easy to read or extend.
> > I don't quite like the tests below, because they refer to objects
> > created a few screens above (tuple_a, key_def_a, etc). Could you
> > please rewrite them in a similar to key_def_new_cases fashion,
> > without referring to any predefined variables?
> 
> It is easy to separate test cases from a testing code in case of one
> function like key_def.new(), but it is not so easy when we need to test
> several functions with different behaviour. So I vote up for inlining
> related data (tuple_a and so on) to test cases, but doubt these cases
> could be written in such declarative manner as I did for key_def.new().

I didn't mean to mix all function test cases in one table. I meant
using a separate table for each function. Something like this:

tuple_compare_test_cases = {
	{
		'Tuple compare with collation',
		parts = {{
			fieldno = 1,
			type = 'string',
			collation = 'unicode_ci',
		}},
		tuple1 = {'test1', 1, 2},
		tuple2 = {'test2', 3},
		exp_err = nil,
		exp_ret = 1,
	}
	...
}

tuple_extract_key_cases = {
	...
}

Do you think it would be an overkill?

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

On Wed, Apr 03, 2019 at 03:01:21PM +0300, Vladimir Davydov wrote:
> On Wed, Apr 03, 2019 at 02:46:44PM +0300, Alexander Turenko wrote:
> > > > +-- Case: extract_key().
> > > > +test:test('extract_key()', function(test)
> > > > +    test:plan(9)
> > > > +
> > > > +    test:is_deeply(key_def_a:extract_key(tuple_a):totable(), {1}, 'case 1')
> > > > +    test:is_deeply(key_def_b:extract_key(tuple_a):totable(), {1, 22}, 'case 2')
> > > > +
> > > > +    -- JSON path.
> > > > +    local res = key_def_lib.new({
> > > > +        {type = 'string', fieldno = 1, path = 'a.b'},
> > > > +    }):extract_key(box.tuple.new({{a = {b = 'foo'}}})):totable()
> > > > +    test:is_deeply(res, {'foo'}, 'JSON path (tuple argument)')
> > > > +
> > > > +    local res = key_def_lib.new({
> > > > +        {type = 'string', fieldno = 1, path = 'a.b'},
> > > > +    }):extract_key({{a = {b = 'foo'}}}):totable()
> > > > +    test:is_deeply(res, {'foo'}, 'JSON path (table argument)')
> > > 
> > > I like key_def_new_cases - they are very easy to read or extend.
> > > I don't quite like the tests below, because they refer to objects
> > > created a few screens above (tuple_a, key_def_a, etc). Could you
> > > please rewrite them in a similar to key_def_new_cases fashion,
> > > without referring to any predefined variables?
> > 
> > It is easy to separate test cases from a testing code in case of one
> > function like key_def.new(), but it is not so easy when we need to test
> > several functions with different behaviour. So I vote up for inlining
> > related data (tuple_a and so on) to test cases, but doubt these cases
> > could be written in such declarative manner as I did for key_def.new().
> 
> I didn't mean to mix all function test cases in one table. I meant
> using a separate table for each function. Something like this:
> 
> tuple_compare_test_cases = {
> 	{
> 		'Tuple compare with collation',
> 		parts = {{
> 			fieldno = 1,
> 			type = 'string',
> 			collation = 'unicode_ci',
> 		}},
> 		tuple1 = {'test1', 1, 2},
> 		tuple2 = {'test2', 3},
> 		exp_err = nil,
> 		exp_ret = 1,
> 	}
> 	...
> }
> 
> tuple_extract_key_cases = {
> 	...
> }
> 
> Do you think it would be an overkill?

It seems this way will look more structured, you are right. I agree, but
don't insist, because the cases are simple (a function call +
is/is_deeply).

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

> > A key_def instance has the following methods:
> > 
> > * :extract_key(tuple)           -> key (as tuple)
> > * :compare(tuple_a, tuple_b)    -> number
> > * :compare_with_key(tuple, key) -> number
> 
> What number do these functions return?
> 
> > * :merge(another_key_def)       -> new key_def instance
> 
> What does 'merge' do?
> 
> > * :totable()                    -> table
> 
> Does this function return key_def parts? In what format?
> Please elaborate the comments.

Note: I think it worth to leave this list of brief descriptions in this
format and describe meaning of arguments and return values for each
function below.

> 
> Also, key_def.compare() sounds like it compares key definitions, not
> tuples. May be, we should move these functions to box.tuple module?

I'm tentative about that. The key_def Lua module is planned to be the
interface to comparators and here we're using comparators. I don't like
spreading of such function across several modules. Maybe 'key_def' name
is not good and we need to use something dedicated from the word
'comparator'?

> 
> Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better
> introduce 'equal', 'greater', 'less', etc helpers returning bool?

A function for table.sort(table, func) returns boolean, so it make
sense. I'm a bit afraid that we'll need to make two calls: say, :less()
and :equal() to determine an order of tuples strictly. But I cannot
provide a case where it can be necessary. Are you know one?

> 
> I'm not strongly against the proposed API, but I think we should agree
> on it with other members of the team, potential users, and Kostja.

I propose to discuss the questions you arose between us and then send
RFC email for the API (something very like docbot comment we already
have).

> > +struct key_def *
> > +check_key_def(struct lua_State *L, int idx)
> 
> Please prefix the name with lbox_ or... I dunno - the naming looks
> inconsistent: luaT_key_def_set_part, lbox_push_key_part, check_key_def.
> Is there some kind of pattern?

I understood the convention so: luaL/luaT is somewhat that operates on a
Lua stack / state, but cannot be called from Lua directly (because
either receive or return C values). So luaT_key_def_set_part() looks
right, but lbox_push_key_part(), lbox_key_def_check_tuple() and
check_key_def() seems to need be prefixed with luaT.

I'll also update check_ibuf(), check_merger_source() and
check_merger_context() in the merger patchset (they are statis however).

> > +/**
> > + * Take existent tuple from LUA stack or build a new tuple with
> > + * default format from table, check for compatibility with a
> > + * given key_def. Take tuple reference pointer on success.
> > + */
> > +static struct tuple *
> > +lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
> > +{
> > +	struct tuple *tuple = luaT_istuple(L, idx);
> > +	if (tuple == NULL)
> > +		tuple = luaT_tuple_new(L, idx, box_tuple_format_default());
> > +	if (tuple == NULL)
> > +		return NULL;
> > +	/* Check that tuple match with the key definition. */
> > +	uint32_t min_field_count =
> > +		tuple_format_min_field_count(&key_def, 1, NULL, 0);
> > +	uint32_t field_count = tuple_field_count(tuple);
> > +	if (field_count < min_field_count) {
> > +		diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1);
> > +		return NULL;
> > +	}
> > +	for (uint32_t idx = 0; idx < key_def->part_count; idx++) {
> > +		struct key_part *part = &key_def->parts[idx];
> > +		const char *field = tuple_field_by_part(tuple, part);
> > +		if (field == NULL) {
> > +			assert(key_def->has_optional_parts);
> > +			continue;
> > +		}
> > +		if (key_part_validate(part->type, field, idx,
> > +				      key_part_is_nullable(part)) != 0)
> > +			return NULL;
> > +	}
> > +	tuple_ref(tuple);
> > +	return tuple;
> > +}
> 
> The code checking a tuple against key_def should live somewhere in
> src/box - chances are high that we miss lua/key_def.c when we extend
> key_def struct again.

Can you suggest where it is better to place this code: src/box/key_def.c
or src/box/tuple.c?

> > +LUA_API int
> > +luaopen_key_def(struct lua_State *L)
> > +{
> > +	luaL_cdef(L, "struct key_def;");
> > +	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
> > +
> > +	/* Export C functions to Lua. */
> > +	static const struct luaL_Reg meta[] = {
> > +		{"new", lbox_key_def_new},
> > +		{NULL, NULL}
> > +	};
> > +	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, "totable");
> > +	lua_setfield(L, -2, "internal");
> 
> Why 'internal'? We use them as is as key_def methods.
> E.g. box.tuple.* methods aren't internal.

To distinguish between module and instance methods and don't confuse a
user with, say, tab completion in a console. fio.c does the same.
However using, say, <luafun iterator>:map(box.tuple.totable) is
convenient, so maybe it worth to name this table, say,
'key_def.instance'?

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Kirill Shcherbatov]

>> The code checking a tuple against key_def should live somewhere in
>> src/box - chances are high that we miss lua/key_def.c when we extend
>> key_def struct again.> 
> Can you suggest where it is better to place this code: src/box/key_def.c
> or src/box/tuple.c?
Due to the fact that this code needs table_field_by_part and
tuple_format_min_field_count, it cannot be placed in key_def.c
Placing it in tuple.h is not quite correct either; that's why we've got it inlined.

>> Probably, we should reuse tuple_validate() for checking a tuple against
>> a key_def so as not to implement the same code again.

Unfortunately tuple_validate() is designed for format validation while we don't
have format here and I don't like create it for validation event in case of error.


As for assertion you've found, that is my fault; required fix doesn't increase
code complexity (not sure about error message):

@@ -243,8 +242,11 @@ lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
 		struct key_part *part = &key_def->parts[idx];
 		const char *field = tuple_field_by_part(tuple, part);
 		if (field == NULL) {
-			assert(key_def->has_optional_parts);
-			continue;
+			if (key_part_is_nullable(part))
+				continue;
+			diag_set(IllegalParams, "tuple doesn't match key "
+						"definition (part %d)", idx);
+			return NULL;
 		}
 		if (key_part_validate(part->type, field, idx,
 				      key_part_is_nullable(part)) != 0)


>> Mixing ER_WRONG_INDEX_OPTIONS and IllegalParams looks ugly. Please fix.

@@ -143,8 +143,7 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *parts,
 		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");
+			diag_set(IllegalParams, "invalid path: \"%s\"", path);
 			return -1;
 		}


I quote Alexander's message because it contains answers and suggestions for other questions.

On 04.04.2019 8:07, Alexander Turenko wrote:
>>> A key_def instance has the following methods:
>>>
>>> * :extract_key(tuple)           -> key (as tuple)
>>> * :compare(tuple_a, tuple_b)    -> number
>>> * :compare_with_key(tuple, key) -> number
>>
>> What number do these functions return?
>>
>>> * :merge(another_key_def)       -> new key_def instance
>>
>> What does 'merge' do?
>>
>>> * :totable()                    -> table
>>
>> Does this function return key_def parts? In what format?
>> Please elaborate the comments.
> 
> Note: I think it worth to leave this list of brief descriptions in this
> format and describe meaning of arguments and return values for each
> function below.
> 
>>
>> Also, key_def.compare() sounds like it compares key definitions, not
>> tuples. May be, we should move these functions to box.tuple module?
> 
> I'm tentative about that. The key_def Lua module is planned to be the
> interface to comparators and here we're using comparators. I don't like
> spreading of such function across several modules. Maybe 'key_def' name
> is not good and we need to use something dedicated from the word
> 'comparator'?
> 
>>
>> Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better
>> introduce 'equal', 'greater', 'less', etc helpers returning bool?
> 
> A function for table.sort(table, func) returns boolean, so it make
> sense. I'm a bit afraid that we'll need to make two calls: say, :less()
> and :equal() to determine an order of tuples strictly. But I cannot
> provide a case where it can be necessary. Are you know one?
> 
>>
>> I'm not strongly against the proposed API, but I think we should agree
>> on it with other members of the team, potential users, and Kostja.
> 
> I propose to discuss the questions you arose between us and then send
> RFC email for the API (something very like docbot comment we already
> have).
> 
>>> +struct key_def *
>>> +check_key_def(struct lua_State *L, int idx)
>>
>> Please prefix the name with lbox_ or... I dunno - the naming looks
>> inconsistent: luaT_key_def_set_part, lbox_push_key_part, check_key_def.
>> Is there some kind of pattern?
> 
> I understood the convention so: luaL/luaT is somewhat that operates on a
> Lua stack / state, but cannot be called from Lua directly (because
> either receive or return C values). So luaT_key_def_set_part() looks
> right, but lbox_push_key_part(), lbox_key_def_check_tuple() and
> check_key_def() seems to need be prefixed with luaT.
> 
> I'll also update check_ibuf(), check_merger_source() and
> check_merger_context() in the merger patchset (they are statis however).
> 
>>> +/**
>>> + * Take existent tuple from LUA stack or build a new tuple with
>>> + * default format from table, check for compatibility with a
>>> + * given key_def. Take tuple reference pointer on success.
>>> + */
>>> +static struct tuple *
>>> +lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
>>> +{
>>> +	struct tuple *tuple = luaT_istuple(L, idx);
>>> +	if (tuple == NULL)
>>> +		tuple = luaT_tuple_new(L, idx, box_tuple_format_default());
>>> +	if (tuple == NULL)
>>> +		return NULL;
>>> +	/* Check that tuple match with the key definition. */
>>> +	uint32_t min_field_count =
>>> +		tuple_format_min_field_count(&key_def, 1, NULL, 0);
>>> +	uint32_t field_count = tuple_field_count(tuple);
>>> +	if (field_count < min_field_count) {
>>> +		diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1);
>>> +		return NULL;
>>> +	}
>>> +	for (uint32_t idx = 0; idx < key_def->part_count; idx++) {
>>> +		struct key_part *part = &key_def->parts[idx];
>>> +		const char *field = tuple_field_by_part(tuple, part);
>>> +		if (field == NULL) {
>>> +			assert(key_def->has_optional_parts);
>>> +			continue;
>>> +		}
>>> +		if (key_part_validate(part->type, field, idx,
>>> +				      key_part_is_nullable(part)) != 0)
>>> +			return NULL;
>>> +	}
>>> +	tuple_ref(tuple);
>>> +	return tuple;
>>> +}
>>
>> The code checking a tuple against key_def should live somewhere in
>> src/box - chances are high that we miss lua/key_def.c when we extend
>> key_def struct again.
> 
> Can you suggest where it is better to place this code: src/box/key_def.c
> or src/box/tuple.c?
> 
>>> +LUA_API int
>>> +luaopen_key_def(struct lua_State *L)
>>> +{
>>> +	luaL_cdef(L, "struct key_def;");
>>> +	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
>>> +
>>> +	/* Export C functions to Lua. */
>>> +	static const struct luaL_Reg meta[] = {
>>> +		{"new", lbox_key_def_new},
>>> +		{NULL, NULL}
>>> +	};
>>> +	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, "totable");
>>> +	lua_setfield(L, -2, "internal");
>>
>> Why 'internal'? We use them as is as key_def methods.
>> E.g. box.tuple.* methods aren't internal.
> 
> To distinguish between module and instance methods and don't confuse a
> user with, say, tab completion in a console. fio.c does the same.
> However using, say, <luafun iterator>:map(box.tuple.totable) is
> convenient, so maybe it worth to name this table, say,
> 'key_def.instance'?

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Vladimir Davydov]

On Thu, Apr 04, 2019 at 11:04:10AM +0300, Kirill Shcherbatov wrote:
> >> The code checking a tuple against key_def should live somewhere in
> >> src/box - chances are high that we miss lua/key_def.c when we extend
> >> key_def struct again.> 
> > Can you suggest where it is better to place this code: src/box/key_def.c
> > or src/box/tuple.c?
> Due to the fact that this code needs table_field_by_part and
> tuple_format_min_field_count, it cannot be placed in key_def.c
> Placing it in tuple.h is not quite correct either; that's why we've got it inlined.

Well, you could put it in tuple_format.c.

> 
> >> Probably, we should reuse tuple_validate() for checking a tuple against
> >> a key_def so as not to implement the same code again.
> 
> Unfortunately tuple_validate() is designed for format validation while we don't
> have format here and I don't like create it for validation event in case of error.

Creating a format on each call to 'compare' is prohibitive from
performance pov, you're right. However, we could attach a format to
Lua's key_def object. This would also speed up comparisons, as tuples
created for this format would have fieldmap.

> 
> 
> As for assertion you've found, that is my fault; required fix doesn't increase
> code complexity (not sure about error message):
> 
> @@ -243,8 +242,11 @@ lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
>  		struct key_part *part = &key_def->parts[idx];
>  		const char *field = tuple_field_by_part(tuple, part);
>  		if (field == NULL) {
> -			assert(key_def->has_optional_parts);
> -			continue;
> +			if (key_part_is_nullable(part))
> +				continue;
> +			diag_set(IllegalParams, "tuple doesn't match key "
> +						"definition (part %d)", idx);
> +			return NULL;

Again, mixing IllegalParams and ClientError (from key_part_validate)
doesn't look good.

Also, after this fix you don't need the min_field_count check.

However, what I don't like is that in case a tuple is created from a Lua
table, you'll have to access all fields twice - on validation and
comparison - which is costly without a fieldmap. One more argument for
attaching a format to Lua's key_def object.

>  		}
>  		if (key_part_validate(part->type, field, idx,
>  				      key_part_is_nullable(part)) != 0)
> 
> 
> >> Mixing ER_WRONG_INDEX_OPTIONS and IllegalParams looks ugly. Please fix.
> 
> @@ -143,8 +143,7 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *parts,
>  		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");
> +			diag_set(IllegalParams, "invalid path: \"%s\"", path);

Then you don't need to pass parts+part_idx to this function -
you can pass only the part you need to set, as it was initially
done by Alexander.

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

> > >> Probably, we should reuse tuple_validate() for checking a tuple against
> > >> a key_def so as not to implement the same code again.
> > 
> > Unfortunately tuple_validate() is designed for format validation while we don't
> > have format here and I don't like create it for validation event in case of error.
> 
> Creating a format on each call to 'compare' is prohibitive from
> performance pov, you're right. However, we could attach a format to
> Lua's key_def object. This would also speed up comparisons, as tuples

So this module will not more be relatively simple layer that exposes
box/key_def into Lua and this again suggests me that the name key_def
maybe is not good choice.

> created for this format would have fieldmap.

Note: It will speed up comparisons only in case when a user give a lua
table as input, not when it give a tuple. However if a user give a tuple
it seems that it is in (s)he responsibility to think about the format.

And, while we're here, maybe it worth to provide a function like
box.tuple.new(), but with key_def instance as an argument to allow a
user to create tuples with necessary formats (to speed up comparisons
and other fields accesses)?

> However, what I don't like is that in case a tuple is created from a Lua
> table, you'll have to access all fields twice - on validation and
> comparison - which is costly without a fieldmap. One more argument for
> attaching a format to Lua's key_def object.

I think we can add *_unchecked() function variants like in
msgpack/msgpackffi if the peformance will be really a problem and
someone will request for them.

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Vladimir Davydov]

On Thu, Apr 04, 2019 at 02:46:22PM +0300, Alexander Turenko wrote:
> > > >> Probably, we should reuse tuple_validate() for checking a tuple against
> > > >> a key_def so as not to implement the same code again.
> > > 
> > > Unfortunately tuple_validate() is designed for format validation while we don't
> > > have format here and I don't like create it for validation event in case of error.
> > 
> > Creating a format on each call to 'compare' is prohibitive from
> > performance pov, you're right. However, we could attach a format to
> > Lua's key_def object. This would also speed up comparisons, as tuples
> 
> So this module will not more be relatively simple layer that exposes
> box/key_def into Lua and this again suggests me that the name key_def
> maybe is not good choice.
> 
> > created for this format would have fieldmap.
> 
> Note: It will speed up comparisons only in case when a user give a lua

Yeah, right. So speeding up comparisons isn't an iron-clad argument
for attaching a format to Lua key_def object. May be not worth the
complexity it will introduce.

> table as input, not when it give a tuple. However if a user give a tuple
> it seems that it is in (s)he responsibility to think about the format.
> 
> And, while we're here, maybe it worth to provide a function like
> box.tuple.new(), but with key_def instance as an argument to allow a
> user to create tuples with necessary formats (to speed up comparisons
> and other fields accesses)?

No, I don't think so: the API wouldn't be generic enough, because one
might want to compare the same tuple using different key definitions.

> 
> > However, what I don't like is that in case a tuple is created from a Lua
> > table, you'll have to access all fields twice - on validation and
> > comparison - which is costly without a fieldmap. One more argument for
> > attaching a format to Lua's key_def object.
> 
> I think we can add *_unchecked() function variants like in
> msgpack/msgpackffi if the peformance will be really a problem and
> someone will request for them.

I don't think that _unchecked (i.e. crash on malformed input) is a good
idea, especially in this case.

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Vladimir Davydov]

On Thu, Apr 04, 2019 at 08:07:34AM +0300, Alexander Turenko wrote:
> > Also, key_def.compare() sounds like it compares key definitions, not
> > tuples. May be, we should move these functions to box.tuple module?
> 
> I'm tentative about that. The key_def Lua module is planned to be the
> interface to comparators and here we're using comparators. I don't like
> spreading of such function across several modules. Maybe 'key_def' name
> is not good and we need to use something dedicated from the word
> 'comparator'?

It's not just a comparator. It's also used for extracting keys.
Regarding moving this code to box.tuple - I don't insist. Let's
just solicit approval from Kostja on this one.

> 
> > 
> > Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better
> > introduce 'equal', 'greater', 'less', etc helpers returning bool?
> 
> A function for table.sort(table, func) returns boolean, so it make
> sense. I'm a bit afraid that we'll need to make two calls: say, :less()
> and :equal() to determine an order of tuples strictly. But I cannot
> provide a case where it can be necessary. Are you know one?

No, I don't write much code in Lua.

However, if we decided to switch to bool return parameter, I'd implement
all possible combinations, i.e. eq, ge, le, gt, lt.

> 
> > 
> > I'm not strongly against the proposed API, but I think we should agree
> > on it with other members of the team, potential users, and Kostja.
> 
> I propose to discuss the questions you arose between us and then send
> RFC email for the API (something very like docbot comment we already
> have).

Agree, an RFC letter with this API and possible alternatives would be
good.

> 
> > > +struct key_def *
> > > +check_key_def(struct lua_State *L, int idx)
> > 
> > Please prefix the name with lbox_ or... I dunno - the naming looks
> > inconsistent: luaT_key_def_set_part, lbox_push_key_part, check_key_def.
> > Is there some kind of pattern?
> 
> I understood the convention so: luaL/luaT is somewhat that operates on a
> Lua stack / state, but cannot be called from Lua directly (because
> either receive or return C values). So luaT_key_def_set_part() looks
> right, but lbox_push_key_part(), lbox_key_def_check_tuple() and
> check_key_def() seems to need be prefixed with luaT.
> 
> I'll also update check_ibuf(), check_merger_source() and
> check_merger_context() in the merger patchset (they are statis however).

Okay. I just want to see some pattern, at least in the scope of the same
source file.

> 
> > > +/**
> > > + * Take existent tuple from LUA stack or build a new tuple with
> > > + * default format from table, check for compatibility with a
> > > + * given key_def. Take tuple reference pointer on success.
> > > + */
> > > +static struct tuple *
> > > +lbox_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx)
> > > +{
> > > +	struct tuple *tuple = luaT_istuple(L, idx);
> > > +	if (tuple == NULL)
> > > +		tuple = luaT_tuple_new(L, idx, box_tuple_format_default());
> > > +	if (tuple == NULL)
> > > +		return NULL;
> > > +	/* Check that tuple match with the key definition. */
> > > +	uint32_t min_field_count =
> > > +		tuple_format_min_field_count(&key_def, 1, NULL, 0);
> > > +	uint32_t field_count = tuple_field_count(tuple);
> > > +	if (field_count < min_field_count) {
> > > +		diag_set(ClientError, ER_NO_SUCH_FIELD_NO, field_count + 1);
> > > +		return NULL;
> > > +	}
> > > +	for (uint32_t idx = 0; idx < key_def->part_count; idx++) {
> > > +		struct key_part *part = &key_def->parts[idx];
> > > +		const char *field = tuple_field_by_part(tuple, part);
> > > +		if (field == NULL) {
> > > +			assert(key_def->has_optional_parts);
> > > +			continue;
> > > +		}
> > > +		if (key_part_validate(part->type, field, idx,
> > > +				      key_part_is_nullable(part)) != 0)
> > > +			return NULL;
> > > +	}
> > > +	tuple_ref(tuple);
> > > +	return tuple;
> > > +}
> > 
> > The code checking a tuple against key_def should live somewhere in
> > src/box - chances are high that we miss lua/key_def.c when we extend
> > key_def struct again.
> 
> Can you suggest where it is better to place this code: src/box/key_def.c
> or src/box/tuple.c?

Actually, I was thinking about reusing tuple_validate() for this to
avoid code duplication. However, if we decide to introduce a separate
function, I guess it should live either in key_def.c or tuple_format.c.

> 
> > > +LUA_API int
> > > +luaopen_key_def(struct lua_State *L)
> > > +{
> > > +	luaL_cdef(L, "struct key_def;");
> > > +	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
> > > +
> > > +	/* Export C functions to Lua. */
> > > +	static const struct luaL_Reg meta[] = {
> > > +		{"new", lbox_key_def_new},
> > > +		{NULL, NULL}
> > > +	};
> > > +	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, "totable");
> > > +	lua_setfield(L, -2, "internal");
> > 
> > Why 'internal'? We use them as is as key_def methods.
> > E.g. box.tuple.* methods aren't internal.
> 
> To distinguish between module and instance methods and don't confuse a

But 'compare', 'merge', etc can be used both as module methods and as
instance method, i.e. neither of the ways of using say compare is wrong:

	k = key_def.new(...)
	k:compare(t1, t2)
	key_def.compare(k, t1, t2)

> user with, say, tab completion in a console. fio.c does the same.

I like to see all public methods suggested to me when I type
key_def.<TAB><TAB>. Don't know about fio, but box.tuple does the same:

	t:update(...)
	box.tuple.update(t, ...)

are equally correct AFAIU.

> However using, say, <luafun iterator>:map(box.tuple.totable) is
> convenient, so maybe it worth to name this table, say,
> 'key_def.instance'?

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

On Thu, Apr 04, 2019 at 11:38:24AM +0300, Vladimir Davydov wrote:
> On Thu, Apr 04, 2019 at 08:07:34AM +0300, Alexander Turenko wrote:
> > > Also, key_def.compare() sounds like it compares key definitions, not
> > > tuples. May be, we should move these functions to box.tuple module?
> > 
> > I'm tentative about that. The key_def Lua module is planned to be the
> > interface to comparators and here we're using comparators. I don't like
> > spreading of such function across several modules. Maybe 'key_def' name
> > is not good and we need to use something dedicated from the word
> > 'comparator'?
> 
> It's not just a comparator. It's also used for extracting keys.
> Regarding moving this code to box.tuple - I don't insist. Let's
> just solicit approval from Kostja on this one.

It is a comparator except extracting keys. Or keys-related functions
except comparing a tuple against a tuple. Now I don't sure.

> > 
> > > 
> > > Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better
> > > introduce 'equal', 'greater', 'less', etc helpers returning bool?
> > 
> > A function for table.sort(table, func) returns boolean, so it make
> > sense. I'm a bit afraid that we'll need to make two calls: say, :less()
> > and :equal() to determine an order of tuples strictly. But I cannot
> > provide a case where it can be necessary. Are you know one?
> 
> No, I don't write much code in Lua.
> 
> However, if we decided to switch to bool return parameter, I'd implement
> all possible combinations, i.e. eq, ge, le, gt, lt.

I mean are there cases when we need to check, say t1 < t2 and if it is
false then check whether t1 == t2 or t1 > t2? In other words, cases when
we need to distinguish all three possible situations. Some algorithms?

I looked throught [1] and it seems it does not contain any reason why
<=> operator was added to C++20 except ones related to comparison
operations autogeneration.

We can just add :cmp() in addition to all other variants, what do you
think?

We can also do the trick: return {eq = <boolean>} from :le() / :ge()
instead of true, but this way looks weird.

[1]: http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0515r0.pdf

> > > > +LUA_API int
> > > > +luaopen_key_def(struct lua_State *L)
> > > > +{
> > > > +	luaL_cdef(L, "struct key_def;");
> > > > +	key_def_type_id = luaL_ctypeid(L, "struct key_def&");
> > > > +
> > > > +	/* Export C functions to Lua. */
> > > > +	static const struct luaL_Reg meta[] = {
> > > > +		{"new", lbox_key_def_new},
> > > > +		{NULL, NULL}
> > > > +	};
> > > > +	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, "totable");
> > > > +	lua_setfield(L, -2, "internal");
> > > 
> > > Why 'internal'? We use them as is as key_def methods.
> > > E.g. box.tuple.* methods aren't internal.
> > 
> > To distinguish between module and instance methods and don't confuse a
> 
> But 'compare', 'merge', etc can be used both as module methods and as
> instance method, i.e. neither of the ways of using say compare is wrong:
> 
> 	k = key_def.new(...)
> 	k:compare(t1, t2)
> 	key_def.compare(k, t1, t2)
> 
> > user with, say, tab completion in a console. fio.c does the same.
> 
> I like to see all public methods suggested to me when I type
> key_def.<TAB><TAB>. Don't know about fio, but box.tuple does the same:
> 
> 	t:update(...)
> 	box.tuple.update(t, ...)
> 
> are equally correct AFAIU.

Hm. I didn't thought about that in this way. Okay.

> 
> > However using, say, <luafun iterator>:map(box.tuple.totable) is
> > convenient, so maybe it worth to name this table, say,
> > 'key_def.instance'?

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Alexander Turenko]

> > > > Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better
> > > > introduce 'equal', 'greater', 'less', etc helpers returning bool?
> > > 
> > > A function for table.sort(table, func) returns boolean, so it make
> > > sense. I'm a bit afraid that we'll need to make two calls: say, :less()
> > > and :equal() to determine an order of tuples strictly. But I cannot
> > > provide a case where it can be necessary. Are you know one?
> > 
> > No, I don't write much code in Lua.
> > 
> > However, if we decided to switch to bool return parameter, I'd implement
> > all possible combinations, i.e. eq, ge, le, gt, lt.
> 
> I mean are there cases when we need to check, say t1 < t2 and if it is
> false then check whether t1 == t2 or t1 > t2? In other words, cases when
> we need to distinguish all three possible situations. Some algorithms?
> 
> I looked throught [1] and it seems it does not contain any reason why
> <=> operator was added to C++20 except ones related to comparison
> operations autogeneration.
> 
> We can just add :cmp() in addition to all other variants, what do you
> think?
> 
> We can also do the trick: return {eq = <boolean>} from :le() / :ge()
> instead of true, but this way looks weird.
> 
> [1]: http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0515r0.pdf

One example that looks valid: search in a binary search tree, where
non-leaf nodes hold elements.

Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module

[Vladimir Davydov]

On Thu, Apr 04, 2019 at 03:00:40PM +0300, Alexander Turenko wrote:
> > > > > Also, returning 1, 0, -1 to Lua looks uncommon. May be, we'd better
> > > > > introduce 'equal', 'greater', 'less', etc helpers returning bool?
> > > > 
> > > > A function for table.sort(table, func) returns boolean, so it make
> > > > sense. I'm a bit afraid that we'll need to make two calls: say, :less()
> > > > and :equal() to determine an order of tuples strictly. But I cannot
> > > > provide a case where it can be necessary. Are you know one?
> > > 
> > > No, I don't write much code in Lua.
> > > 
> > > However, if we decided to switch to bool return parameter, I'd implement
> > > all possible combinations, i.e. eq, ge, le, gt, lt.
> > 
> > I mean are there cases when we need to check, say t1 < t2 and if it is
> > false then check whether t1 == t2 or t1 > t2? In other words, cases when
> > we need to distinguish all three possible situations. Some algorithms?
> > 
> > I looked throught [1] and it seems it does not contain any reason why
> > <=> operator was added to C++20 except ones related to comparison
> > operations autogeneration.
> > 
> > We can just add :cmp() in addition to all other variants, what do you
> > think?
> > 
> > We can also do the trick: return {eq = <boolean>} from :le() / :ge()
> > instead of true, but this way looks weird.
> > 
> > [1]: http://open-std.org/JTC1/SC22/WG21/docs/papers/2017/p0515r0.pdf
> 
> One example that looks valid: search in a binary search tree, where
> non-leaf nodes hold elements.

Okay, I see, it means that someone might actually need cmp(), not
eq/le/etc, in which case adding all those methods returning bool doesn't
look like a good idea. I tend to think that the API proposed originally
was okay, after all. Let's send an RFC to dev@ and see what others
think.