* [tarantool-patches] [PATCH v2 0/2] lua: add key_def lua module
@ 2019-03-27 14:29 Kirill Shcherbatov
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new() Kirill Shcherbatov
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 2/2] lua: add key_def lua module Kirill Shcherbatov
0 siblings, 2 replies; 25+ messages in thread
From: Kirill Shcherbatov @ 2019-03-27 14:29 UTC (permalink / raw)
To: tarantool-patches, alexander.turenko; +Cc: 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()
2019-03-27 14:29 [tarantool-patches] [PATCH v2 0/2] lua: add key_def lua module Kirill Shcherbatov
@ 2019-03-27 14:29 ` Kirill Shcherbatov
2019-03-28 9:01 ` [tarantool-patches] " Konstantin Osipov
2019-04-03 18:01 ` [tarantool-patches] " Vladimir Davydov
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 2/2] lua: add key_def lua module Kirill Shcherbatov
1 sibling, 2 replies; 25+ messages in thread
From: Kirill Shcherbatov @ 2019-03-27 14:29 UTC (permalink / raw)
To: tarantool-patches, alexander.turenko
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [tarantool-patches] [PATCH v2 2/2] lua: add key_def lua module
2019-03-27 14:29 [tarantool-patches] [PATCH v2 0/2] lua: add key_def lua module Kirill Shcherbatov
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new() Kirill Shcherbatov
@ 2019-03-27 14:29 ` Kirill Shcherbatov
2019-03-28 2:01 ` Alexander Turenko
1 sibling, 1 reply; 25+ messages in thread
From: Kirill Shcherbatov @ 2019-03-27 14:29 UTC (permalink / raw)
To: tarantool-patches, alexander.turenko; +Cc: 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v2 2/2] lua: add key_def lua module
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 2/2] lua: add key_def lua module Kirill Shcherbatov
@ 2019-03-28 2:01 ` Alexander Turenko
2019-03-28 7:38 ` [tarantool-patches] " Kirill Shcherbatov
2019-03-28 8:41 ` Kirill Shcherbatov
0 siblings, 2 replies; 25+ messages in thread
From: Alexander Turenko @ 2019-03-28 2:01 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches, Vladimir Davydov
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).
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-03-28 2:01 ` Alexander Turenko
@ 2019-03-28 7:38 ` Kirill Shcherbatov
2019-03-28 8:41 ` Kirill Shcherbatov
1 sibling, 0 replies; 25+ messages in thread
From: Kirill Shcherbatov @ 2019-03-28 7:38 UTC (permalink / raw)
To: tarantool-patches, Alexander Turenko; +Cc: Vladimir Davydov
> 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);
^ permalink raw reply [flat|nested] 25+ messages in thread
* [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-03-28 2:01 ` Alexander Turenko
2019-03-28 7:38 ` [tarantool-patches] " Kirill Shcherbatov
@ 2019-03-28 8:41 ` Kirill Shcherbatov
[not found] ` <6d915212-e80f-4a6d-d884-b838bf25f8a7@tarantool.org>
1 sibling, 1 reply; 25+ messages in thread
From: Kirill Shcherbatov @ 2019-03-28 8:41 UTC (permalink / raw)
To: Alexander Turenko, Tarantool MailList
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/2] lua: add luaT_tuple_new()
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new() Kirill Shcherbatov
@ 2019-03-28 9:01 ` Konstantin Osipov
2019-03-28 9:18 ` Alexander Turenko
2019-04-03 18:01 ` [tarantool-patches] " Vladimir Davydov
1 sibling, 1 reply; 25+ messages in thread
From: Konstantin Osipov @ 2019-03-28 9:01 UTC (permalink / raw)
To: tarantool-patches; +Cc: alexander.turenko
* 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/2] lua: add luaT_tuple_new()
2019-03-28 9:01 ` [tarantool-patches] " Konstantin Osipov
@ 2019-03-28 9:18 ` Alexander Turenko
0 siblings, 0 replies; 25+ messages in thread
From: Alexander Turenko @ 2019-03-28 9:18 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
[not found] ` <6d915212-e80f-4a6d-d884-b838bf25f8a7@tarantool.org>
@ 2019-03-28 11:22 ` Alexander Turenko
2019-04-03 11:10 ` Vladimir Davydov
0 siblings, 1 reply; 25+ messages in thread
From: Alexander Turenko @ 2019-03-28 11:22 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches, Kirill Shcherbatov
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
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-03-28 11:22 ` Alexander Turenko
@ 2019-04-03 11:10 ` Vladimir Davydov
2019-04-03 11:46 ` Alexander Turenko
2019-04-04 5:07 ` Alexander Turenko
0 siblings, 2 replies; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-03 11:10 UTC (permalink / raw)
To: Alexander Turenko, Kirill Shcherbatov; +Cc: tarantool-patches
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-03 11:10 ` Vladimir Davydov
@ 2019-04-03 11:46 ` Alexander Turenko
2019-04-03 12:01 ` Vladimir Davydov
2019-04-04 5:07 ` Alexander Turenko
1 sibling, 1 reply; 25+ messages in thread
From: Alexander Turenko @ 2019-04-03 11:46 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Kirill Shcherbatov, tarantool-patches
> > +-- 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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-03 11:46 ` Alexander Turenko
@ 2019-04-03 12:01 ` Vladimir Davydov
2019-04-03 13:26 ` Alexander Turenko
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-03 12:01 UTC (permalink / raw)
To: Alexander Turenko; +Cc: Kirill Shcherbatov, tarantool-patches
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?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-03 12:01 ` Vladimir Davydov
@ 2019-04-03 13:26 ` Alexander Turenko
0 siblings, 0 replies; 25+ messages in thread
From: Alexander Turenko @ 2019-04-03 13:26 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Kirill Shcherbatov, tarantool-patches
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).
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new() Kirill Shcherbatov
2019-03-28 9:01 ` [tarantool-patches] " Konstantin Osipov
@ 2019-04-03 18:01 ` Vladimir Davydov
2019-04-04 2:51 ` Alexander Turenko
1 sibling, 1 reply; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-03 18:01 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches, alexander.turenko
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()
2019-04-03 18:01 ` [tarantool-patches] " Vladimir Davydov
@ 2019-04-04 2:51 ` Alexander Turenko
2019-04-04 8:14 ` Vladimir Davydov
0 siblings, 1 reply; 25+ messages in thread
From: Alexander Turenko @ 2019-04-04 2:51 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Kirill Shcherbatov, tarantool-patches
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-03 11:10 ` Vladimir Davydov
2019-04-03 11:46 ` Alexander Turenko
@ 2019-04-04 5:07 ` Alexander Turenko
2019-04-04 8:04 ` Kirill Shcherbatov
2019-04-04 8:38 ` Vladimir Davydov
1 sibling, 2 replies; 25+ messages in thread
From: Alexander Turenko @ 2019-04-04 5:07 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Kirill Shcherbatov, tarantool-patches
> > 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'?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 5:07 ` Alexander Turenko
@ 2019-04-04 8:04 ` Kirill Shcherbatov
2019-04-04 9:05 ` Vladimir Davydov
2019-04-04 8:38 ` Vladimir Davydov
1 sibling, 1 reply; 25+ messages in thread
From: Kirill Shcherbatov @ 2019-04-04 8:04 UTC (permalink / raw)
To: tarantool-patches, Alexander Turenko, Vladimir Davydov
>> 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'?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new()
2019-04-04 2:51 ` Alexander Turenko
@ 2019-04-04 8:14 ` Vladimir Davydov
0 siblings, 0 replies; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-04 8:14 UTC (permalink / raw)
To: Alexander Turenko; +Cc: Kirill Shcherbatov, tarantool-patches
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 5:07 ` Alexander Turenko
2019-04-04 8:04 ` Kirill Shcherbatov
@ 2019-04-04 8:38 ` Vladimir Davydov
2019-04-04 11:17 ` Alexander Turenko
1 sibling, 1 reply; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-04 8:38 UTC (permalink / raw)
To: Alexander Turenko; +Cc: Kirill Shcherbatov, tarantool-patches
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'?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 8:04 ` Kirill Shcherbatov
@ 2019-04-04 9:05 ` Vladimir Davydov
2019-04-04 11:46 ` Alexander Turenko
0 siblings, 1 reply; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-04 9:05 UTC (permalink / raw)
To: Kirill Shcherbatov; +Cc: tarantool-patches, Alexander Turenko
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 8:38 ` Vladimir Davydov
@ 2019-04-04 11:17 ` Alexander Turenko
2019-04-04 12:00 ` Alexander Turenko
0 siblings, 1 reply; 25+ messages in thread
From: Alexander Turenko @ 2019-04-04 11:17 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Kirill Shcherbatov, tarantool-patches
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'?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 9:05 ` Vladimir Davydov
@ 2019-04-04 11:46 ` Alexander Turenko
2019-04-04 14:36 ` Vladimir Davydov
0 siblings, 1 reply; 25+ messages in thread
From: Alexander Turenko @ 2019-04-04 11:46 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Kirill Shcherbatov, tarantool-patches
> > >> 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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 11:17 ` Alexander Turenko
@ 2019-04-04 12:00 ` Alexander Turenko
2019-04-04 14:42 ` Vladimir Davydov
0 siblings, 1 reply; 25+ messages in thread
From: Alexander Turenko @ 2019-04-04 12:00 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Kirill Shcherbatov, tarantool-patches
> > > > 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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 11:46 ` Alexander Turenko
@ 2019-04-04 14:36 ` Vladimir Davydov
0 siblings, 0 replies; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-04 14:36 UTC (permalink / raw)
To: Alexander Turenko; +Cc: Kirill Shcherbatov, tarantool-patches
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v2 2/2] lua: add key_def lua module
2019-04-04 12:00 ` Alexander Turenko
@ 2019-04-04 14:42 ` Vladimir Davydov
0 siblings, 0 replies; 25+ messages in thread
From: Vladimir Davydov @ 2019-04-04 14:42 UTC (permalink / raw)
To: Alexander Turenko; +Cc: Kirill Shcherbatov, tarantool-patches
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.
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2019-04-04 14:42 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-27 14:29 [tarantool-patches] [PATCH v2 0/2] lua: add key_def lua module Kirill Shcherbatov
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 1/2] lua: add luaT_tuple_new() Kirill Shcherbatov
2019-03-28 9:01 ` [tarantool-patches] " Konstantin Osipov
2019-03-28 9:18 ` Alexander Turenko
2019-04-03 18:01 ` [tarantool-patches] " Vladimir Davydov
2019-04-04 2:51 ` Alexander Turenko
2019-04-04 8:14 ` Vladimir Davydov
2019-03-27 14:29 ` [tarantool-patches] [PATCH v2 2/2] lua: add key_def lua module Kirill Shcherbatov
2019-03-28 2:01 ` Alexander Turenko
2019-03-28 7:38 ` [tarantool-patches] " Kirill Shcherbatov
2019-03-28 8:41 ` Kirill Shcherbatov
[not found] ` <6d915212-e80f-4a6d-d884-b838bf25f8a7@tarantool.org>
2019-03-28 11:22 ` Alexander Turenko
2019-04-03 11:10 ` Vladimir Davydov
2019-04-03 11:46 ` Alexander Turenko
2019-04-03 12:01 ` Vladimir Davydov
2019-04-03 13:26 ` Alexander Turenko
2019-04-04 5:07 ` Alexander Turenko
2019-04-04 8:04 ` Kirill Shcherbatov
2019-04-04 9:05 ` Vladimir Davydov
2019-04-04 11:46 ` Alexander Turenko
2019-04-04 14:36 ` Vladimir Davydov
2019-04-04 8:38 ` Vladimir Davydov
2019-04-04 11:17 ` Alexander Turenko
2019-04-04 12:00 ` Alexander Turenko
2019-04-04 14:42 ` Vladimir Davydov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox