* [Tarantool-patches] [PATCH v3 01/16] module api: get rid of typedef redefinitions
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 02/16] module api: expose box region Alexander Turenko
` (17 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
Technically C99 forbids it. Clang complains about typedef redefinitions
when C99 is used (it is default prior to clang 3.6):
| error: redefinition of typedef 'box_tuple_t' is a C11 feature
| [-Werror,-Wtypedef-redefinition]
| error: redefinition of typedef 'box_key_def_t' is a C11 feature
| [-Werror,-Wtypedef-redefinition]
The generated module.h file should define a type using typedef once.
This patch moves extra definitions out of public parts of header files.
Reordered api headers to place usages of such types after definitions.
Set C99 for the module API test in order to catch problems of this kind
in a future. Fixed 'unused value' warnings, which appears after the
change (it is strange that -Wall was not passed here before).
Fixes #5313
---
src/CMakeLists.txt | 2 +-
src/box/index.h | 5 +++--
src/box/key_def.h | 3 ++-
test/app-tap/CMakeLists.txt | 6 ++++++
test/app-tap/module_api.c | 2 ++
5 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 68d69eded..699536652 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -144,10 +144,10 @@ set(api_headers
${CMAKE_SOURCE_DIR}/src/lua/error.h
${CMAKE_SOURCE_DIR}/src/lua/string.h
${CMAKE_SOURCE_DIR}/src/box/txn.h
+ ${CMAKE_SOURCE_DIR}/src/box/tuple.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
${CMAKE_SOURCE_DIR}/src/box/tuple_extract_key.h
${CMAKE_SOURCE_DIR}/src/box/schema_def.h
diff --git a/src/box/index.h b/src/box/index.h
index 86148023f..6225a8674 100644
--- a/src/box/index.h
+++ b/src/box/index.h
@@ -46,10 +46,11 @@ struct index_def;
struct key_def;
struct info_handler;
-/** \cond public */
-
typedef struct tuple box_tuple_t;
typedef struct key_def box_key_def_t;
+
+/** \cond public */
+
typedef struct iterator box_iterator_t;
/**
diff --git a/src/box/key_def.h b/src/box/key_def.h
index f4d9e76f2..2826273cc 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -287,10 +287,11 @@ key_def_copy(struct key_def *dest, const struct key_def *src);
void
key_def_delete(struct key_def *def);
+typedef struct tuple box_tuple_t;
+
/** \cond public */
typedef struct key_def box_key_def_t;
-typedef struct tuple box_tuple_t;
/**
* Create key definition with key fields with passed typed on passed positions.
diff --git a/test/app-tap/CMakeLists.txt b/test/app-tap/CMakeLists.txt
index bf7d28136..0fb34711e 100644
--- a/test/app-tap/CMakeLists.txt
+++ b/test/app-tap/CMakeLists.txt
@@ -1,2 +1,8 @@
build_module(module_api module_api.c)
build_module(libyield libyield.c)
+
+# gh-5313: verify that module.h actually conforms to the C99
+# standard.
+set(CMAKE_C_FLAGS "-Wall -Wextra -std=c99")
+set(CMAKE_C_FLAGS_DEBUG "-g -O0 -Werror")
+set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2")
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index b81a98056..a79fbed0d 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -198,6 +198,7 @@ test_toint64(lua_State *L)
int fiber_test_func(va_list va)
{
+ (void) va;
do {
fiber_set_cancellable(true);
fiber_sleep(0.01);
@@ -380,6 +381,7 @@ test_call(lua_State *L)
static int
cpcall_handler(lua_State *L)
{
+ (void) L;
return 0;
}
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 02/16] module api: expose box region
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 01/16] module api: get rid of typedef redefinitions Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 03/16] module api/lua: add luaL_iscdata() function Alexander Turenko
` (16 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
It is the better alternative to linking the small library directly to a
module. Why not just use the small library in a module?
Functions from an executable are preferred over ones that are shipped in
a dynamic library (on Linux, Mac OS differs), while a module may use the
small library directly. It may cause a problem when some functions from
the library are inlined, but some are not, and those different versions
of the library offer structures with different layouts. Small library
symbols may be exported by the tarantool executable after the change of
default symbols visibility (see [1]). See more details and examples in
[2].
So it is better to expose so called box region and get rid of all those
compatibility problems.
[1]: 2.5.0-42-g03790ac55 ('cmake: remove dynamic-list linker option')
[2]: https://lists.tarantool.org/pipermail/tarantool-discussions/2020-September/000095.html
Part of #5273
---
src/exports.h | 4 ++
src/lib/core/fiber.c | 24 +++++++++
src/lib/core/fiber.h | 76 +++++++++++++++++++++++++++++
test/app-tap/module_api.c | 84 ++++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 189 insertions(+), 1 deletion(-)
diff --git a/src/exports.h b/src/exports.h
index 6d8303180..7861bb529 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -218,6 +218,10 @@ EXPORT(fiber_is_cancelled)
EXPORT(fiber_join)
EXPORT(fiber_new)
EXPORT(fiber_new_ex)
+EXPORT(box_region_aligned_alloc)
+EXPORT(box_region_alloc)
+EXPORT(box_region_truncate)
+EXPORT(box_region_used)
EXPORT(fiber_reschedule)
EXPORT(fiber_self)
EXPORT(fiber_set_cancellable)
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 223c841df..33ec47c66 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1368,6 +1368,30 @@ fiber_top_disable(void)
}
#endif /* ENABLE_FIBER_TOP */
+size_t
+box_region_used(void)
+{
+ return region_used(&fiber()->gc);
+}
+
+void *
+box_region_alloc(size_t size)
+{
+ return region_alloc(&fiber()->gc, size);
+}
+
+void *
+box_region_aligned_alloc(size_t size, size_t alignment)
+{
+ return region_aligned_alloc(&fiber()->gc, size, alignment);
+}
+
+void
+box_region_truncate(size_t size)
+{
+ return region_truncate(&fiber()->gc, size);
+}
+
void
cord_create(struct cord *cord, const char *name)
{
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index 16ee9f414..a3014cc0a 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -386,6 +386,82 @@ struct slab_cache;
API_EXPORT struct slab_cache *
cord_slab_cache(void);
+/**
+ * box region allocator
+ *
+ * It is the region allocator from the small library. It is useful
+ * for allocating tons of small objects and free them at once.
+ *
+ * Typical usage is illustrated in the sketch below.
+ *
+ * | size_t region_svp = box_region_used();
+ * | while (<...>) {
+ * | char *buf = box_region_alloc(<...>);
+ * | <...>
+ * | }
+ * | box_region_truncate(region_svp);
+ *
+ * There are module API functions that return a result on
+ * this region. In this case a module is responsible to free the
+ * result:
+ *
+ * | size_t region_svp = box_region_used();
+ * | char *buf = box_<...>(<...>);
+ * | <...>
+ * | box_region_truncate(region_svp);
+ *
+ * This API provides better compatibility guarantees over using
+ * the small library directly in a module. A binary layout of
+ * internal structures may be changed in a future, but
+ * <box_region_*>() functions will remain API and ABI compatible.
+ *
+ * Data allocated on the region are guaranteed to be valid until
+ * a fiber yield or a call of a function from the certain set:
+ *
+ * - Related to transactions.
+ * - Ones that may cause box initialization (box.cfg()).
+ * - Ones that may involve SQL execution.
+ *
+ * FIXME: Provide more strict list of functions, which may
+ * invalidate the data: ones that may lead to calling of
+ * fiber_gc().
+ *
+ * It is safe to call simple box APIs around tuples, key_defs and
+ * so on -- they don't invalidate the allocated data.
+ *
+ * Don't assume this region as fiber local. This is an
+ * implementation detail and may be changed in a future.
+ */
+
+/** How much memory is used by the box region. */
+API_EXPORT size_t
+box_region_used(void);
+
+/**
+ * Allocate size bytes from the box region.
+ *
+ * Don't use this function to allocate a memory block for a value
+ * or array of values of a type with alignment requirements. A
+ * violation of alignment requirements leads to undefined
+ * behaviour.
+ */
+API_EXPORT void *
+box_region_alloc(size_t size);
+
+/**
+ * Allocate size bytes from the box region with given alignment.
+ *
+ * Alignment must be a power of 2.
+ */
+API_EXPORT void *
+box_region_aligned_alloc(size_t size, size_t alignment);
+
+/**
+ * Truncate the box region to the given size.
+ */
+void
+box_region_truncate(size_t size);
+
/** \endcond public */
/**
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index a79fbed0d..12d20e886 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -15,6 +15,10 @@
#define STR2(x) #x
#define STR(x) STR2(x)
+#ifndef lengthof
+#define lengthof(array) (sizeof(array) / sizeof((array)[0]))
+#endif
+
/* Test for constants */
static const char *consts[] = {
PACKAGE_VERSION,
@@ -451,6 +455,85 @@ test_iscallable(lua_State *L)
return 1;
}
+/* {{{ test_box_region */
+
+/**
+ * Verify basic usage of box region.
+ */
+static int
+test_box_region(struct lua_State *L)
+{
+ size_t region_svp_0 = box_region_used();
+
+ /* Verify allocation and box_region_used(). */
+ size_t size_arr[] = {1, 7, 19, 10 * 1024 * 1024, 1, 18, 1024};
+ size_t region_svp_arr[lengthof(size_arr)];
+ char *ptr_arr[lengthof(size_arr)];
+ for (size_t i = 0; i < lengthof(size_arr); ++i) {
+ size_t size = size_arr[i];
+ size_t region_svp = box_region_used();
+ char *ptr = box_region_alloc(size);
+
+ /* Verify box_region_used() after allocation. */
+ assert(box_region_used() - region_svp == size);
+
+ /* Verify that data is accessible. */
+ for (char *p = ptr; p < ptr + size; ++p)
+ *p = 'x';
+
+ /*
+ * Save data pointer and savepoint to verify
+ * truncation later.
+ */
+ ptr_arr[i] = ptr;
+ region_svp_arr[i] = region_svp;
+ }
+
+ /* Verify truncation. */
+ for (ssize_t i = lengthof(region_svp_arr) - 1; i >= 0; --i) {
+ box_region_truncate(region_svp_arr[i]);
+ assert(box_region_used() == region_svp_arr[i]);
+
+ /*
+ * Verify that all data before this savepoint
+ * still accessible.
+ */
+ for (ssize_t j = 0; j < i; ++j) {
+ size_t size = size_arr[j];
+ char *ptr = ptr_arr[j];
+ for (char *p = ptr; p < ptr + size; ++p) {
+ assert(*p == 'x' || *p == 'y');
+ *p = 'y';
+ }
+ }
+ }
+ assert(box_region_used() == region_svp_0);
+
+ /* Verify aligned allocation. */
+ size_t a_size_arr[] = {1, 3, 5, 7, 11, 13, 17, 19};
+ size_t alignment_arr[] = {1, 2, 4, 8, 16, 32, 64};
+ for (size_t s = 0; s < lengthof(a_size_arr); ++s) {
+ for (size_t a = 0; a < lengthof(alignment_arr); ++a) {
+ size_t size = a_size_arr[s];
+ size_t alignment = alignment_arr[a];
+ char *ptr = box_region_aligned_alloc(size, alignment);
+ assert((uintptr_t) ptr % alignment == 0);
+
+ /* Data is accessible. */
+ for (char *p = ptr; p < ptr + size; ++p)
+ *p = 'x';
+ }
+ }
+
+ /* Clean up. */
+ box_region_truncate(region_svp_0);
+
+ lua_pushboolean(L, true);
+ return 1;
+}
+
+/* }}} test_box_region */
+
LUA_API int
luaopen_module_api(lua_State *L)
{
@@ -479,6 +562,7 @@ luaopen_module_api(lua_State *L)
{"test_state", test_state},
{"test_tostring", test_tostring},
{"iscallable", test_iscallable},
+ {"test_box_region", test_box_region},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index a6658cc61..08e8add35 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -117,7 +117,7 @@ local function test_iscallable(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(24)
+ test:plan(25)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 02/16] module api: expose box region
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 02/16] module api: expose box region Alexander Turenko
@ 2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-15 13:17 ` Alexander Turenko
0 siblings, 1 reply; 33+ messages in thread
From: Vladislav Shpilevoy @ 2020-10-14 23:41 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Thanks for the patch!
See 3 comments below.
> diff --git a/src/exports.h b/src/exports.h
> index 6d8303180..7861bb529 100644
> --- a/src/exports.h
> +++ b/src/exports.h
> @@ -218,6 +218,10 @@ EXPORT(fiber_is_cancelled)
> EXPORT(fiber_join)
> EXPORT(fiber_new)
> EXPORT(fiber_new_ex)
> +EXPORT(box_region_aligned_alloc)
> +EXPORT(box_region_alloc)
> +EXPORT(box_region_truncate)
> +EXPORT(box_region_used)
1. Could you please keep the function list sorted?
'b' < 'f'.
2. Don't we want to also export reserve/aligned_reserve,
while we are here? They are useful when need to write data
knowing its maximal size, and then call alloc() with the
exact size in the end.
> diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> index 16ee9f414..a3014cc0a 100644
> --- a/src/lib/core/fiber.h
> +++ b/src/lib/core/fiber.h
> @@ -386,6 +386,82 @@ struct slab_cache;
> +/**
> + * Allocate size bytes from the box region.
> + *
> + * Don't use this function to allocate a memory block for a value
> + * or array of values of a type with alignment requirements. A
> + * violation of alignment requirements leads to undefined
> + * behaviour.
> + */
> +API_EXPORT void *
> +box_region_alloc(size_t size);
> +
> +/**
> + * Allocate size bytes from the box region with given alignment.
> + *
> + * Alignment must be a power of 2.
> + */
> +API_EXPORT void *
> +box_region_aligned_alloc(size_t size, size_t alignment);
> +
> +/**
> + * Truncate the box region to the given size.
> + */
> +void
3. + API_EXPORT.
> +box_region_truncate(size_t size);
> +
> /** \endcond public */
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 02/16] module api: expose box region
2020-10-14 23:41 ` Vladislav Shpilevoy
@ 2020-10-15 13:17 ` Alexander Turenko
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-15 13:17 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Thu, Oct 15, 2020 at 01:41:41AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 3 comments below.
>
> > diff --git a/src/exports.h b/src/exports.h
> > index 6d8303180..7861bb529 100644
> > --- a/src/exports.h
> > +++ b/src/exports.h
> > @@ -218,6 +218,10 @@ EXPORT(fiber_is_cancelled)
> > EXPORT(fiber_join)
> > EXPORT(fiber_new)
> > EXPORT(fiber_new_ex)
> > +EXPORT(box_region_aligned_alloc)
> > +EXPORT(box_region_alloc)
> > +EXPORT(box_region_truncate)
> > +EXPORT(box_region_used)
>
> 1. Could you please keep the function list sorted?
> 'b' < 'f'.
Ouch, sorry for this. Fixed on the branch.
>
> 2. Don't we want to also export reserve/aligned_reserve,
> while we are here? They are useful when need to write data
> knowing its maximal size, and then call alloc() with the
> exact size in the end.
I don't like to do something in a hurry and without proper documentation
and test.
It surely worth it, so included into my backlog.
>
> > diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
> > index 16ee9f414..a3014cc0a 100644
> > --- a/src/lib/core/fiber.h
> > +++ b/src/lib/core/fiber.h
> > @@ -386,6 +386,82 @@ struct slab_cache;
> > +/**
> > + * Allocate size bytes from the box region.
> > + *
> > + * Don't use this function to allocate a memory block for a value
> > + * or array of values of a type with alignment requirements. A
> > + * violation of alignment requirements leads to undefined
> > + * behaviour.
> > + */
> > +API_EXPORT void *
> > +box_region_alloc(size_t size);
> > +
> > +/**
> > + * Allocate size bytes from the box region with given alignment.
> > + *
> > + * Alignment must be a power of 2.
> > + */
> > +API_EXPORT void *
> > +box_region_aligned_alloc(size_t size, size_t alignment);
> > +
> > +/**
> > + * Truncate the box region to the given size.
> > + */
> > +void
>
> 3. + API_EXPORT.
Fixed on the branches.
>
> > +box_region_truncate(size_t size);
> > +
> > /** \endcond public */
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Tarantool-patches] [PATCH v3 03/16] module api/lua: add luaL_iscdata() function
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 01/16] module api: get rid of typedef redefinitions Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 02/16] module api: expose box region Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 04/16] lua: factor out tuple encoding from luaT_tuple_new Alexander Turenko
` (15 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
It is useful to provide a module specific error when cdata expected, but
a value of another type is passed.
Alternative would be using of lua_type() to check against LUA_TCDATA,
but this constant is not exposed for modules. See more in the
luaL_iscdata() API comment.
Part of #5273
---
src/exports.h | 1 +
src/lua/utils.c | 6 +++
src/lua/utils.h | 20 ++++++++++
test/app-tap/module_api.c | 22 +++++++++++
test/app-tap/module_api.test.lua | 63 +++++++++++++++++++++++++++++++-
5 files changed, 111 insertions(+), 1 deletion(-)
diff --git a/src/exports.h b/src/exports.h
index 7861bb529..01c1aa83e 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -356,6 +356,7 @@ EXPORT(luaL_findtable)
EXPORT(luaL_getmetafield)
EXPORT(luaL_gsub)
EXPORT(luaL_iscallable)
+EXPORT(luaL_iscdata)
EXPORT(luaL_loadbuffer)
EXPORT(luaL_loadbufferx)
EXPORT(luaL_loadfile)
diff --git a/src/lua/utils.c b/src/lua/utils.c
index 399bec6c6..3e1c491f8 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -113,6 +113,12 @@ luaL_pushuuid(struct lua_State *L)
return luaL_pushcdata(L, CTID_UUID);
}
+int
+luaL_iscdata(struct lua_State *L, int idx)
+{
+ return lua_type(L, idx) == LUA_TCDATA;
+}
+
void *
luaL_checkcdata(struct lua_State *L, int idx, uint32_t *ctypeid)
{
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 7e02a05f2..e80e2b1a2 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -78,6 +78,26 @@ luaL_pushuuid(struct lua_State *L);
/** \cond public */
+/**
+ * Checks whether a value on the Lua stack is a cdata.
+ *
+ * Unlike <luaL_checkcdata>() this function does not raise an
+ * error. It is useful to raise a domain specific error.
+ *
+ * Lua API and module API don't expose LUA_TCDATA constant.
+ * We have no guarantee that this constant will remain the same in
+ * future LuaJIT versions. So this function should be used in
+ * modules instead of `lua_type(L, idx) == LUA_TCDATA`.
+ *
+ * @param L Lua state.
+ * @param idx Acceptable index on the Lua stack.
+ *
+ * @retval 1 If the value at the given index is a cdata.
+ * @retval 0 Otherwise.
+ */
+LUA_API int
+luaL_iscdata(struct lua_State *L, int idx);
+
/**
* @brief Push cdata of given \a ctypeid onto the stack.
* CTypeID must be used from FFI at least once. Allocated memory returned
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 12d20e886..fa5388ba0 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -455,6 +455,27 @@ test_iscallable(lua_State *L)
return 1;
}
+static int
+test_iscdata(struct lua_State *L)
+{
+ assert(lua_gettop(L) == 2);
+
+ int exp = lua_toboolean(L, 2);
+
+ /* Basic test. */
+ int res = luaL_iscdata(L, 1);
+ int ok = res == exp;
+ assert(lua_gettop(L) == 2);
+
+ /* Use negative index. */
+ res = luaL_iscdata(L, -2);
+ ok = ok && res == exp;
+ assert(lua_gettop(L) == 2);
+
+ lua_pushboolean(L, res == exp);
+ return 1;
+}
+
/* {{{ test_box_region */
/**
@@ -562,6 +583,7 @@ luaopen_module_api(lua_State *L)
{"test_state", test_state},
{"test_tostring", test_tostring},
{"iscallable", test_iscallable},
+ {"iscdata", test_iscdata},
{"test_box_region", test_box_region},
{NULL, NULL}
};
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 08e8add35..0231dc3b0 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -116,8 +116,68 @@ local function test_iscallable(test, module)
end
end
+local function test_iscdata(test, module)
+ local ffi = require('ffi')
+ ffi.cdef([[
+ struct foo { int bar; };
+ ]])
+
+ local cases = {
+ {
+ obj = nil,
+ exp = false,
+ description = 'nil',
+ },
+ {
+ obj = 1,
+ exp = false,
+ description = 'number',
+ },
+ {
+ obj = 'hello',
+ exp = false,
+ description = 'string',
+ },
+ {
+ obj = {},
+ exp = false,
+ description = 'table',
+ },
+ {
+ obj = function() end,
+ exp = false,
+ description = 'function',
+ },
+ {
+ obj = ffi.new('struct foo'),
+ exp = true,
+ description = 'cdata',
+ },
+ {
+ obj = ffi.new('struct foo *'),
+ exp = true,
+ description = 'cdata pointer',
+ },
+ {
+ obj = ffi.new('struct foo &'),
+ exp = true,
+ description = 'cdata reference',
+ },
+ {
+ obj = 1LL,
+ exp = true,
+ description = 'cdata number',
+ },
+ }
+
+ test:plan(#cases)
+ for _, case in ipairs(cases) do
+ test:ok(module.iscdata(case.obj, case.exp), case.description)
+ end
+end
+
local test = require('tap').test("module_api", function(test)
- test:plan(25)
+ test:plan(26)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
@@ -143,6 +203,7 @@ local test = require('tap').test("module_api", function(test)
test:test("pushcdata", test_pushcdata, module)
test:test("iscallable", test_iscallable, module)
+ test:test("iscdata", test_iscdata, module)
space:drop()
end)
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 04/16] lua: factor out tuple encoding from luaT_tuple_new
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (2 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 03/16] module api/lua: add luaL_iscdata() function Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 05/16] lua: don't raise a Lua error from luaT_tuple_new() Alexander Turenko
` (14 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
It simplifies further changes around encoding a tuple: next commits will
get rid of throwing serialization errors and will expose a function to
encode a table (or tuple) from the Lua stack on the box region to the
module API.
Part of #5273
Part of #5382
---
src/box/lua/tuple.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 03b4b8a2e..ed97c85e4 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -98,8 +98,9 @@ luaT_istuple(struct lua_State *L, int narg)
return *(struct tuple **) data;
}
-struct tuple *
-luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
+static char *
+luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
+ size_t *tuple_len_ptr)
{
if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
diag_set(IllegalParams, "A tuple or a table expected, got %s",
@@ -127,8 +128,20 @@ luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
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));
+ if (tuple_len_ptr != NULL)
+ *tuple_len_ptr = ibuf_used(buf);
+ return buf->buf;
+}
+
+struct tuple *
+luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
+{
+ size_t tuple_len;
+ char *tuple_data = luaT_tuple_encode_on_lua_ibuf(L, idx, &tuple_len);
+ if (tuple_data == NULL)
+ return NULL;
+ struct tuple *tuple = box_tuple_new(format, tuple_data,
+ tuple_data + tuple_len);
if (tuple == NULL)
return NULL;
ibuf_reinit(tarantool_lua_ibuf);
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 05/16] lua: don't raise a Lua error from luaT_tuple_new()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (3 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 04/16] lua: factor out tuple encoding from luaT_tuple_new Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 06/16] module api/lua: add luaT_tuple_encode() Alexander Turenko
` (13 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
This change fixes incorrect behaviour at tuple serialization error in
several places: <space_object>:frommap(), <key_def_object>:compare(),
<merge_source>:select(). See more in #5382.
Disallow creating a tuple from objects on the Lua stack (idx == 0) in
luaT_tuple_new() for simplicity. There are no such usages in tarantool.
The function is not exposed yet to the module API. This is only
necessary in box.tuple.new(), which anyway raises Lua errors by its
contract.
The better way to implement it would be rewritting of serialization from
Lua to msgpack without raising Lua errors, but it is labirous work. Some
day, I hope, I'll return here.
Part of #5273
Fixes #5382
---
src/box/lua/tuple.c | 124 ++++++++++++++++++++++++--------
src/box/lua/tuple.h | 7 +-
test/unit/luaT_tuple_new.c | 42 ++++++++---
test/unit/luaT_tuple_new.result | 6 +-
4 files changed, 134 insertions(+), 45 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index ed97c85e4..8c3d29f71 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
uint32_t CTID_STRUCT_TUPLE_REF;
+static int luaT_tuple_encode_table_ref = LUA_NOREF;
+
box_tuple_t *
luaT_checktuple(struct lua_State *L, int idx)
{
@@ -98,39 +100,88 @@ luaT_istuple(struct lua_State *L, int narg)
return *(struct tuple **) data;
}
+/**
+ * Encode a Lua values on a Lua stack as an MsgPack array.
+ *
+ * Raise a Lua error when encoding fails.
+ *
+ * Helper for <lbox_tuple_new>().
+ */
+static int
+luaT_tuple_encode_values(struct lua_State *L)
+{
+ 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);
+ int argc = lua_gettop(L);
+ mpstream_encode_array(&stream, argc);
+ for (int k = 1; k <= argc; ++k) {
+ luamp_encode(L, luaL_msgpack_default, NULL, &stream, k);
+ }
+ mpstream_flush(&stream);
+ return 0;
+}
+
+/**
+ * Encode a Lua table or a tuple as MsgPack.
+ *
+ * Raise a Lua error when encoding fails.
+ *
+ * It is a kind of critical section to be run under luaT_call().
+ */
+static int
+luaT_tuple_encode_table(struct lua_State *L)
+{
+ 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);
+ luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
+ mpstream_flush(&stream);
+ return 0;
+}
+
+/**
+ * Encode a Lua table / tuple to Lua shared ibuf.
+ */
static char *
luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
size_t *tuple_len_ptr)
{
- if (idx != 0 && !lua_istable(L, idx) && !luaT_istuple(L, idx)) {
+ assert(idx != 0);
+ if (!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;
- ibuf_reset(buf);
- struct mpstream stream;
- mpstream_init(&stream, buf, ibuf_reserve_cb, ibuf_alloc_cb,
- luamp_error, L);
- 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, NULL, &stream, k);
- }
- } else {
- /* Create the tuple from a Lua table. */
- luamp_encode_tuple(L, &tuple_serializer, &stream, idx);
- }
- mpstream_flush(&stream);
+ /* To restore before leaving the function. */
+ int top = lua_gettop(L);
+
+ /*
+ * An absolute index doesn't need to be recalculated after
+ * the stack size change.
+ */
+ if (idx < 0)
+ idx = top + idx + 1;
+
+ assert(luaT_tuple_encode_table_ref != LUA_NOREF);
+ lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
+ assert(lua_isfunction(L, -1));
+
+ lua_pushvalue(L, idx);
+
+ int rc = luaT_call(L, 1, 0);
+ lua_settop(L, top);
+ if (rc != 0)
+ return NULL;
+
if (tuple_len_ptr != NULL)
- *tuple_len_ptr = ibuf_used(buf);
- return buf->buf;
+ *tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);
+ return tarantool_lua_ibuf->buf;
}
struct tuple *
@@ -156,15 +207,29 @@ lbox_tuple_new(lua_State *L)
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).
+ * box.tuple.new(1, 2, 3).
*/
- 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 (argc != 1 || (!lua_istable(L, 1) && !luaT_istuple(L, 1))) {
+ struct ibuf *buf = tarantool_lua_ibuf;
+ luaT_tuple_encode_values(L); /* may raise */
+ struct tuple *tuple = box_tuple_new(fmt, buf->buf,
+ buf->buf + ibuf_used(buf));
+ ibuf_reinit(buf);
+ if (tuple == NULL)
+ return luaT_error(L);
+ luaT_pushtuple(L, tuple);
+ return 1;
+ }
+
+ /*
+ * Use the new parameters format:
+ * box.tuple.new({1, 2, 3}).
+ */
+ struct tuple *tuple = luaT_tuple_new(L, 1, fmt);
if (tuple == NULL)
return luaT_error(L);
/* box_tuple_new() doesn't leak on exception, see public API doc */
@@ -593,4 +658,7 @@ box_lua_tuple_init(struct lua_State *L)
(void) rc;
CTID_STRUCT_TUPLE_REF = luaL_ctypeid(L, "struct tuple &");
assert(CTID_STRUCT_TUPLE_REF != 0);
+
+ lua_pushcfunction(L, luaT_tuple_encode_table);
+ luaT_tuple_encode_table_ref = luaL_ref(L, LUA_REGISTRYINDEX);
}
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index fd280f565..6787d1afe 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -82,11 +82,8 @@ luaT_istuple(struct lua_State *L, int idx);
/** \endcond public */
/**
- * 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.
+ * Create a new tuple with specific format from a Lua table or a
+ * tuple.
*
* In case of an error set a diag and return NULL.
*/
diff --git a/test/unit/luaT_tuple_new.c b/test/unit/luaT_tuple_new.c
index 8f25c8e07..09a81cabe 100644
--- a/test/unit/luaT_tuple_new.c
+++ b/test/unit/luaT_tuple_new.c
@@ -51,20 +51,20 @@ check_tuple(struct tuple *tuple, box_tuple_format_t *format,
void
check_error(struct lua_State *L, struct tuple *tuple, int retvals,
+ const struct type_info *error_type, const char *exp_err,
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);
+ is(e->type, error_type, "%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);
+ plan(23);
header();
int top;
@@ -106,14 +106,12 @@ test_basic(struct lua_State *L)
assert(lua_gettop(L) == 0);
/*
- * Case: elements on the stack (idx == 0) as an input and
- * a non-default format.
+ * Case: a non-default format (a Lua table on idx == -1).
*/
/* Prepare the Lua stack. */
- lua_pushinteger(L, 1);
- lua_pushinteger(L, 2);
- lua_pushinteger(L, 3);
+ luaL_loadstring(L, "return {1, 2, 3}");
+ lua_call(L, 0, 1);
/* Create a new format. */
struct key_part_def part;
@@ -130,12 +128,12 @@ test_basic(struct lua_State *L)
/* Create and check a tuple. */
top = lua_gettop(L);
- tuple = luaT_tuple_new(L, 0, another_format);
+ tuple = luaT_tuple_new(L, -1, another_format);
check_tuple(tuple, another_format, lua_gettop(L) - top, "objects");
/* Clean up. */
tuple_format_delete(another_format);
- lua_pop(L, 3);
+ lua_pop(L, 1);
assert(lua_gettop(L) == 0);
/*
@@ -148,7 +146,28 @@ test_basic(struct lua_State *L)
/* 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");
+ check_error(L, tuple, lua_gettop(L) - top, &type_IllegalParams,
+ "A tuple or a table expected, got number",
+ "unexpected type");
+
+ /* Clean up. */
+ lua_pop(L, 1);
+ assert(lua_gettop(L) == 0);
+
+ /*
+ * Case: unserializable item within a Lua table.
+ *
+ * The function should not raise a Lua error.
+ */
+ luaL_loadstring(L, "return {function() end}");
+ lua_call(L, 0, 1);
+
+ /* 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, &type_LuajitError,
+ "unsupported Lua type 'function'",
+ "unserializable element");
/* Clean up. */
lua_pop(L, 1);
@@ -170,6 +189,7 @@ main()
luaL_openlibs(L);
box_init();
+ tarantool_lua_error_init(L);
luaopen_msgpack(L);
box_lua_tuple_init(L);
lua_pop(L, 1);
diff --git a/test/unit/luaT_tuple_new.result b/test/unit/luaT_tuple_new.result
index 110aa68c2..8f3407130 100644
--- a/test/unit/luaT_tuple_new.result
+++ b/test/unit/luaT_tuple_new.result
@@ -1,4 +1,4 @@
-1..19
+1..23
*** test_basic ***
ok 1 - table: tuple != NULL
ok 2 - table: check tuple format id
@@ -19,4 +19,8 @@ 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
+ok 20 - unserializable element: tuple == NULL
+ok 21 - unserializable element: check retvals count
+ok 22 - unserializable element: check error type
+ok 23 - unserializable element: check error message
*** test_basic: done ***
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 05/16] lua: don't raise a Lua error from luaT_tuple_new()
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 05/16] lua: don't raise a Lua error from luaT_tuple_new() Alexander Turenko
@ 2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-15 13:17 ` Alexander Turenko
0 siblings, 1 reply; 33+ messages in thread
From: Vladislav Shpilevoy @ 2020-10-14 23:41 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Thanks for the patch!
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index ed97c85e4..8c3d29f71 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
>
> uint32_t CTID_STRUCT_TUPLE_REF;
>
> +static int luaT_tuple_encode_table_ref = LUA_NOREF;
This probably needs a comment.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 05/16] lua: don't raise a Lua error from luaT_tuple_new()
2020-10-14 23:41 ` Vladislav Shpilevoy
@ 2020-10-15 13:17 ` Alexander Turenko
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-15 13:17 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Thu, Oct 15, 2020 at 01:41:47AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> > diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> > index ed97c85e4..8c3d29f71 100644
> > --- a/src/box/lua/tuple.c
> > +++ b/src/box/lua/tuple.c
> > @@ -69,6 +69,8 @@ extern char tuple_lua[]; /* Lua source */
> >
> > uint32_t CTID_STRUCT_TUPLE_REF;
> >
> > +static int luaT_tuple_encode_table_ref = LUA_NOREF;
>
> This probably needs a comment.
Added on the branches:
+/**
+ * <luaT_tuple_encode_table>() reference in the Lua registry.
+ *
+ * Storing of the reference allows to don't create a new GCfunc
+ * object each time we call the function in the protected mode.
+ * It reduces Lua GC pressure in comparison with calling of
+ * <lua_cpcall>() or <lua_pushcfunction>() on each invocation.
+ */
static int luaT_tuple_encode_table_ref = LUA_NOREF;
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Tarantool-patches] [PATCH v3 06/16] module api/lua: add luaT_tuple_encode()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (4 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 05/16] lua: don't raise a Lua error from luaT_tuple_new() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 07/16] module api/lua: expose luaT_tuple_new() Alexander Turenko
` (12 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
It is the same as luaT_tuple_new(), but returns raw MsgPack data (not
<box_tuple_t>) allocated on the box region.
The reason to expose this function is to provide ability to use
box_tuple_compare_with_key() function from an external module for a key
passed as a Lua table. The compare function has the following signature:
| API_EXPORT int
| box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
| box_key_def_t *key_def);
The second parameter is a key encoded as an MsgPack array, not a tuple
structure. So luaT_tuple_new() is not applicable here (it is not
worthful to create a tuple structure if we need just MsgPack data).
Some complexity was introduced to support encoding on the Lua shared
buffer and the box region both. The motivation is the following:
- luaT_tuple_encode() is exposed with encoding to the box region,
because it is more usual to the module API. In particular a user of
the API able to control when the tuple data should be released.
- Encoding to the Lua shared buffer is kept internally, because there is
no strong reason to change it to the box region for box.tuple.new().
Part of #5273
---
src/box/lua/tuple.c | 101 +++++++++++++++++++++++----
src/box/lua/tuple.h | 17 +++++
src/exports.h | 1 +
test/app-tap/module_api.c | 115 +++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 221 insertions(+), 15 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 8c3d29f71..191e84539 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -100,6 +100,25 @@ luaT_istuple(struct lua_State *L, int narg)
return *(struct tuple **) data;
}
+/* {{{ Encode a Lua table as an MsgPack array */
+
+/*
+ * A lot of functions are there, however the task per se looks
+ * simple. Reasons are the following.
+ *
+ * 1. box.tuple.new() supports two parameters conventions.
+ * <luaT_tuple_encode_values>() implements the old API.
+ * 2. Serializer from Lua to MsgPack may raise a Lua error,
+ * so it should be run under pcall. The dangerous code is
+ * encapsulated into <luaT_tuple_encode_table>().
+ * 3. In particular <mpstream_init>() may raise an error in case
+ * of OOM, so it also is run under pcall.
+ * 4. box.tuple.new() and <luaT_tuple_new>() use shared Lua ibuf
+ * under the hood (because there is no strong reason to change
+ * it), while <luaT_tuple_encode>() uses the box region
+ * (because it is usual for the module API).
+ */
+
/**
* Encode a Lua values on a Lua stack as an MsgPack array.
*
@@ -124,6 +143,22 @@ luaT_tuple_encode_values(struct lua_State *L)
return 0;
}
+typedef void luaT_mpstream_init_f(struct mpstream *stream, struct lua_State *L);
+
+static void
+luaT_mpstream_init_lua_ibuf(struct mpstream *stream, struct lua_State *L)
+{
+ mpstream_init(stream, tarantool_lua_ibuf, ibuf_reserve_cb,
+ ibuf_alloc_cb, luamp_error, L);
+}
+
+static void
+luaT_mpstream_init_box_region(struct mpstream *stream, struct lua_State *L)
+{
+ mpstream_init(stream, &fiber()->gc, region_reserve_cb, region_alloc_cb,
+ luamp_error, L);
+}
+
/**
* Encode a Lua table or a tuple as MsgPack.
*
@@ -134,28 +169,26 @@ luaT_tuple_encode_values(struct lua_State *L)
static int
luaT_tuple_encode_table(struct lua_State *L)
{
- 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);
- luamp_encode_tuple(L, &tuple_serializer, &stream, 1);
+ luaT_mpstream_init_f *luaT_mpstream_init_f = lua_topointer(L, 1);
+ luaT_mpstream_init_f(&stream, L);
+ luamp_encode_tuple(L, &tuple_serializer, &stream, 2);
mpstream_flush(&stream);
return 0;
}
/**
- * Encode a Lua table / tuple to Lua shared ibuf.
+ * Encode a Lua table / tuple to given mpstream.
*/
-static char *
-luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
- size_t *tuple_len_ptr)
+static int
+luaT_tuple_encode_on_mpstream(struct lua_State *L, int idx,
+ luaT_mpstream_init_f *luaT_mpstream_init_f)
{
assert(idx != 0);
if (!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;
+ return -1;
}
/* To restore before leaving the function. */
@@ -172,18 +205,58 @@ luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
lua_rawgeti(L, LUA_REGISTRYINDEX, luaT_tuple_encode_table_ref);
assert(lua_isfunction(L, -1));
+ lua_pushlightuserdata(L, luaT_mpstream_init_f);
lua_pushvalue(L, idx);
- int rc = luaT_call(L, 1, 0);
+ int rc = luaT_call(L, 2, 0);
lua_settop(L, top);
- if (rc != 0)
+ return rc;
+}
+
+/**
+ * Encode a Lua table / tuple to Lua shared ibuf.
+ */
+static char *
+luaT_tuple_encode_on_lua_ibuf(struct lua_State *L, int idx,
+ size_t *tuple_len_ptr)
+{
+ struct ibuf *buf = tarantool_lua_ibuf;
+ ibuf_reset(buf);
+ if (luaT_tuple_encode_on_mpstream(L, idx,
+ luaT_mpstream_init_lua_ibuf) != 0)
return NULL;
+ if (tuple_len_ptr != NULL)
+ *tuple_len_ptr = ibuf_used(buf);
+ return buf->buf;
+}
+/**
+ * Encode a Lua table / tuple to box region.
+ */
+char *
+luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr)
+{
+ struct region *region = &fiber()->gc;
+ size_t region_svp = region_used(region);
+ if (luaT_tuple_encode_on_mpstream(L, idx,
+ luaT_mpstream_init_box_region) != 0) {
+ region_truncate(region, region_svp);
+ return NULL;
+ }
+ size_t tuple_len = region_used(region) - region_svp;
if (tuple_len_ptr != NULL)
- *tuple_len_ptr = ibuf_used(tarantool_lua_ibuf);
- return tarantool_lua_ibuf->buf;
+ *tuple_len_ptr = tuple_len;
+ char *tuple_data = region_join(region, tuple_len);
+ if (tuple_data == NULL) {
+ diag_set(OutOfMemory, tuple_len, "region", "tuple data");
+ region_truncate(region, region_svp);
+ return NULL;
+ }
+ return tuple_data;
}
+/* }}} Encode a Lua table as an MsgPack array */
+
struct tuple *
luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
{
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index 6787d1afe..aadcf7f59 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -31,6 +31,7 @@
* SUCH DAMAGE.
*/
#include <stddef.h>
+#include "trivia/util.h"
#if defined(__cplusplus)
extern "C" {
@@ -79,6 +80,22 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
box_tuple_t *
luaT_istuple(struct lua_State *L, int idx);
+/**
+ * Encode a table or a tuple on the Lua stack as an MsgPack array.
+ *
+ * @param L Lua state.
+ * @param idx Acceptable index on the Lua stack.
+ * @param tuple_len_ptr Where to store tuple data size in bytes
+ * (or NULL).
+ *
+ * The storage for data is allocated on the box region. A caller
+ * should call <box_region_truncate>() to release the data.
+ *
+ * In case of an error set a diag and return NULL.
+ */
+API_EXPORT char *
+luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr);
+
/** \endcond public */
/**
diff --git a/src/exports.h b/src/exports.h
index 01c1aa83e..7bdba5693 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -406,6 +406,7 @@ EXPORT(luaT_istuple)
EXPORT(luaT_pushtuple)
EXPORT(luaT_state)
EXPORT(luaT_tolstring)
+EXPORT(luaT_tuple_encode)
EXPORT(mp_char2escape)
EXPORT(mp_decode_double)
EXPORT(mp_decode_extl)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index fa5388ba0..0090a35a5 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -555,6 +555,120 @@ test_box_region(struct lua_State *L)
/* }}} test_box_region */
+/* {{{ test_tuple_encode */
+
+static void
+check_tuple_data(char *tuple_data, size_t tuple_size, int retvals)
+{
+ assert(tuple_size == 4);
+ assert(tuple_data != NULL);
+ assert(!strncmp(tuple_data, "\x93\x01\x02\x03", 4));
+ assert(retvals == 0);
+}
+
+static void
+check_encode_error(char *tuple_data, int retvals, const char *exp_err_type,
+ const char *exp_err_msg)
+{
+ assert(tuple_data == NULL);
+ box_error_t *e = box_error_last();
+ assert(strcmp(box_error_type(e), exp_err_type) == 0);
+ assert(strcmp(box_error_message(e), exp_err_msg) == 0);
+ assert(retvals == 0);
+}
+
+/**
+ * Encode a Lua table or a tuple into a tuple.
+ *
+ * Similar to <luaT_tuple_new>() unit test.
+ */
+static int
+test_tuple_encode(struct lua_State *L)
+{
+ int top;
+ char *tuple_data;
+ size_t tuple_size;
+
+ size_t region_svp = box_region_used();
+
+ /*
+ * 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_data = luaT_tuple_encode(L, -2, &tuple_size);
+ check_tuple_data(tuple_data, tuple_size, lua_gettop(L) - top);
+
+ /* Clean up. */
+ lua_pop(L, 2);
+ assert(lua_gettop(L) == 0);
+
+ /*
+ * Case: a tuple on idx == -1 as an input.
+ */
+
+ /* Prepare the Lua stack. */
+ luaL_loadstring(L, "return box.tuple.new({1, 2, 3})");
+ lua_call(L, 0, 1);
+
+ /* Create and check a tuple. */
+ top = lua_gettop(L);
+ tuple_data = luaT_tuple_encode(L, -1, &tuple_size);
+ check_tuple_data(tuple_data, tuple_size, lua_gettop(L) - top);
+
+ /* Clean up. */
+ lua_pop(L, 1);
+ assert(lua_gettop(L) == 0);
+
+ /*
+ * Case: a Lua object of an unexpected type.
+ */
+
+ /* Prepare the Lua stack. */
+ lua_pushinteger(L, 42);
+
+ /* Try to encode and check for the error. */
+ top = lua_gettop(L);
+ tuple_data = luaT_tuple_encode(L, -1, &tuple_size);
+ check_encode_error(tuple_data, lua_gettop(L) - top, "IllegalParams",
+ "A tuple or a table expected, got number");
+
+ /* Clean up. */
+ lua_pop(L, 1);
+ assert(lua_gettop(L) == 0);
+
+ /*
+ * Case: unserializable item within a Lua table.
+ *
+ * The function should not raise a Lua error.
+ */
+ luaL_loadstring(L, "return {function() end}");
+ lua_call(L, 0, 1);
+
+ /* Try to encode and check for the error. */
+ top = lua_gettop(L);
+ tuple_data = luaT_tuple_encode(L, -1, &tuple_size);
+ check_encode_error(tuple_data, lua_gettop(L) - top, "LuajitError",
+ "unsupported Lua type 'function'");
+
+ /* Clean up. */
+ lua_pop(L, 1);
+ assert(lua_gettop(L) == 0);
+
+ box_region_truncate(region_svp);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
+/* }}} test_tuple_encode */
+
LUA_API int
luaopen_module_api(lua_State *L)
{
@@ -585,6 +699,7 @@ luaopen_module_api(lua_State *L)
{"iscallable", test_iscallable},
{"iscdata", test_iscdata},
{"test_box_region", test_box_region},
+ {"test_tuple_encode", test_tuple_encode},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 0231dc3b0..e5cac9eb6 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(26)
+ test:plan(27)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 07/16] module api/lua: expose luaT_tuple_new()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (5 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 06/16] module api/lua: add luaT_tuple_encode() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 08/16] module api/lua: add API_EXPORT to tuple functions Alexander Turenko
` (11 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
It is convenient wrapper around box_tuple_new() to create a tuple from a
Lua table (or from another tuple).
Part of #5273
---
src/box/lua/tuple.c | 6 ++---
src/box/lua/tuple.h | 20 +++++++++++++---
src/exports.h | 1 +
test/app-tap/module_api.c | 40 ++++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 62 insertions(+), 7 deletions(-)
diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 191e84539..255fd0bc9 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -257,15 +257,15 @@ luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr)
/* }}} Encode a Lua table as an MsgPack array */
-struct tuple *
+box_tuple_t *
luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format)
{
size_t tuple_len;
char *tuple_data = luaT_tuple_encode_on_lua_ibuf(L, idx, &tuple_len);
if (tuple_data == NULL)
return NULL;
- struct tuple *tuple = box_tuple_new(format, tuple_data,
- tuple_data + tuple_len);
+ box_tuple_t *tuple = box_tuple_new(format, tuple_data,
+ tuple_data + tuple_len);
if (tuple == NULL)
return NULL;
ibuf_reinit(tarantool_lua_ibuf);
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index aadcf7f59..f6384ddb8 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -92,21 +92,35 @@ luaT_istuple(struct lua_State *L, int idx);
* should call <box_region_truncate>() to release the data.
*
* In case of an error set a diag and return NULL.
+ *
+ * @sa luaT_tuple_new()
*/
API_EXPORT char *
luaT_tuple_encode(struct lua_State *L, int idx, size_t *tuple_len_ptr);
-/** \endcond public */
-
/**
* Create a new tuple with specific format from a Lua table or a
* tuple.
*
+ * The new tuple is referenced in the same way as one created by
+ * <box_tuple_new>(). There are two possible usage scenarious:
+ *
+ * 1. A short living tuple may not be referenced explicitly and
+ * will be collected automatically at the next module API call
+ * that yields or returns a tuple.
+ * 2. A long living tuple must be referenced using
+ * <box_tuple_ref>() and unreferenced then with
+ * <box_tuple_unref>().
+ *
+ * @sa box_tuple_ref()
+ *
* In case of an error set a diag and return NULL.
*/
-struct tuple *
+API_EXPORT box_tuple_t *
luaT_tuple_new(struct lua_State *L, int idx, box_tuple_format_t *format);
+/** \endcond public */
+
static inline int
luaT_pushtupleornil(struct lua_State *L, struct tuple *tuple)
{
diff --git a/src/exports.h b/src/exports.h
index 7bdba5693..299bb9fc1 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -407,6 +407,7 @@ EXPORT(luaT_pushtuple)
EXPORT(luaT_state)
EXPORT(luaT_tolstring)
EXPORT(luaT_tuple_encode)
+EXPORT(luaT_tuple_new)
EXPORT(mp_char2escape)
EXPORT(mp_decode_double)
EXPORT(mp_decode_extl)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 0090a35a5..d7304f86d 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -669,6 +669,45 @@ test_tuple_encode(struct lua_State *L)
/* }}} test_tuple_encode */
+/* {{{ test_tuple_new */
+
+/**
+ * Create a tuple from a Lua table or another tuple.
+ *
+ * Just basic test. More cases in the luaT_tuple_new.c unit test.
+ */
+static int
+test_tuple_new(struct lua_State *L)
+{
+ box_tuple_format_t *default_format = box_tuple_format_default();
+
+ /* Prepare the Lua stack. */
+ luaL_loadstring(L, "return {1, 2, 3}");
+ lua_call(L, 0, 1);
+
+ /* Create a tuple. */
+ int top = lua_gettop(L);
+ box_tuple_t *tuple = luaT_tuple_new(L, -1, default_format);
+
+ /* Verify size, data and Lua stack top. */
+ size_t region_svp = box_region_used();
+ size_t tuple_size = box_tuple_bsize(tuple);
+ char *tuple_data = box_region_alloc(tuple_size);
+ ssize_t rc = box_tuple_to_buf(tuple, tuple_data, tuple_size);
+ assert(rc == (ssize_t) tuple_size);
+ check_tuple_data(tuple_data, tuple_size, lua_gettop(L) - top);
+
+ /* Clean up. */
+ box_region_truncate(region_svp);
+ lua_pop(L, 1);
+ assert(lua_gettop(L) == 0);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
+/* }}} test_tuple_new */
+
LUA_API int
luaopen_module_api(lua_State *L)
{
@@ -700,6 +739,7 @@ luaopen_module_api(lua_State *L)
{"iscdata", test_iscdata},
{"test_box_region", test_box_region},
{"test_tuple_encode", test_tuple_encode},
+ {"test_tuple_new", test_tuple_new},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index e5cac9eb6..f1b377e17 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(27)
+ test:plan(28)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 08/16] module api/lua: add API_EXPORT to tuple functions
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (6 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 07/16] module api/lua: expose luaT_tuple_new() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 09/16] module api: add API_EXPORT to key_def functions Alexander Turenko
` (10 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
The reason is unification of declarations. It is the rule of thumb to
use API_EXPORT with module API functions.
Part of #5273
---
src/box/lua/tuple.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/box/lua/tuple.h b/src/box/lua/tuple.h
index f6384ddb8..cf88074b9 100644
--- a/src/box/lua/tuple.h
+++ b/src/box/lua/tuple.h
@@ -57,7 +57,7 @@ typedef struct tuple_format box_tuple_format_t;
* @retval non-NULL argument is tuple
* @throws error if the argument is not a tuple.
*/
-box_tuple_t *
+API_EXPORT box_tuple_t *
luaT_checktuple(struct lua_State *L, int idx);
/**
@@ -66,7 +66,7 @@ luaT_checktuple(struct lua_State *L, int idx);
* @sa luaT_istuple
* @throws on OOM
*/
-void
+API_EXPORT void
luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
/**
@@ -77,7 +77,7 @@ luaT_pushtuple(struct lua_State *L, box_tuple_t *tuple);
* @retval non-NULL argument is tuple
* @retval NULL argument is not tuple
*/
-box_tuple_t *
+API_EXPORT box_tuple_t *
luaT_istuple(struct lua_State *L, int idx);
/**
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 08/16] module api/lua: add API_EXPORT to tuple functions
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 08/16] module api/lua: add API_EXPORT to tuple functions Alexander Turenko
@ 2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-15 2:35 ` Alexander Turenko
0 siblings, 1 reply; 33+ messages in thread
From: Vladislav Shpilevoy @ 2020-10-14 23:41 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Thanks for the patch!
On 13.10.2020 01:23, Alexander Turenko wrote:
> The reason is unification of declarations. It is the rule of thumb to
> use API_EXPORT with module API functions.
What about box_tuple_ref, box_tuple_unref, box_tuple_field_count,
and other tuple functions?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 08/16] module api/lua: add API_EXPORT to tuple functions
2020-10-14 23:41 ` Vladislav Shpilevoy
@ 2020-10-15 2:35 ` Alexander Turenko
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-15 2:35 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Thu, Oct 15, 2020 at 01:41:53AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> On 13.10.2020 01:23, Alexander Turenko wrote:
> > The reason is unification of declarations. It is the rule of thumb to
> > use API_EXPORT with module API functions.
>
> What about box_tuple_ref, box_tuple_unref, box_tuple_field_count,
> and other tuple functions?
My motivation is to don't have mix of functions with and without
API_EXPORT after my changes. In the patchset I added new functions into
box/key_def and box/lua/tuple, so added the macro here. But I don't
touch box/tuple, so don't do any changes for consistency there.
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Tarantool-patches] [PATCH v3 09/16] module api: add API_EXPORT to key_def functions
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (7 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 08/16] module api/lua: add API_EXPORT to tuple functions Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 10/16] module api: add box_key_def_new_v2() Alexander Turenko
` (9 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
It is the rule of thumb to use API_EXPORT with module API functions.
To be honest, I don't see strict reason to use this macro except for
unification with other module API functions.
It adds 'extern', which is default for functions.
It adds __attribute__((visibility("default"))), but symbols to be
exported are listed in extra/exports or src/export.h (depending of
tarantool version, see [1]).
It adds __attribute__((nothrow)), which maybe allows a compiler to
produce more optimized code and also catch an obvious problem and emit a
warning. I don't know.
So the reason for me is unification.
Part of #5273
[1]: 2.5.0-42-g03790ac55 ('cmake: remove dynamic-list linker option')
[2]: 1.6.8-71-g55605c5c9 ('Add __attribute__((nothrow)) to API_EXPORT macro')
---
src/box/key_def.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 2826273cc..625bb6fea 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -302,7 +302,7 @@ typedef struct key_def box_key_def_t;
* \param part_count the number of key fields
* \returns a new key definition object
*/
-box_key_def_t *
+API_EXPORT box_key_def_t *
box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
/**
@@ -310,7 +310,7 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
*
* \param key_def key definition to delete
*/
-void
+API_EXPORT void
box_key_def_delete(box_key_def_t *key_def);
/**
@@ -322,7 +322,7 @@ box_key_def_delete(box_key_def_t *key_def);
* @retval <0 if key_fields(tuple_a) < key_fields(tuple_b)
* @retval >0 if key_fields(tuple_a) > key_fields(tuple_b)
*/
-int
+API_EXPORT int
box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
box_key_def_t *key_def);
@@ -337,7 +337,7 @@ box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
* @retval >0 if key_fields(tuple) > parts(key)
*/
-int
+API_EXPORT int
box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
box_key_def_t *key_def);
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 10/16] module api: add box_key_def_new_v2()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (8 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 09/16] module api: add API_EXPORT to key_def functions Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 11/16] module api: add box_key_def_dump_parts() Alexander Turenko
` (8 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
Unlike box_key_def_new() it allows to set nullability, collation and
JSON path.
Note: JSON paths are not supported in the backported version of the
patch for 1.10.
Provided public non-opaque key part definition structure to create a key
definition. The next commit will also use this structure to dump a key
definition.
There are several technical points around the box_key_part_def_t
structure. They are mainly about providing stable ABI.
- Two uint32_t fields are placed first for better aligning of next
fields (pointers, which are usually 64 bit wide).
- A padding is used to guarantee that the structure size will remain the
same across tarantool versions. It allows to allocate an array of such
structures.
- The padding array is not a field of the structure itself, but added as
a union variant (see the code). It allows to get rid of manual
calculation of cumulative fields size, which is hard to do in a
platform independent way.
- A minimal size of the structure is guaranteed by the union with
padding, but a static assert is required to guarantee that the size
will not overrun the predefined value.
- PACKED is added as an extra remedy to make the structure layout
predictable (on given target architecture).
- A bit flag is used for is_nullable. bool is considered as too
expensive (it requires 8 bits). bitfields (int:1 and so on) do no
guarantee certain data layout (it is compiler implementation detail),
while a module is compiled outside of tarantool build and may use
different toolchain. A bit flag is the only choice.
- A collation is identified using a string. Different IDs may be used on
different tarantool instances for collations. The only 'real'
identifier is a collation name, so using it as identifier in the API
should be more convenient and less error-prone.
- A field type is also identified using a string instead of a number. We
have <enum field_type> in the module API, but it cannot be used here,
because IDs are changed across tarantool versions. Aside of this, size
of a enum is compiler defined. Anyway, we can expose field types as
numbers and implement number-to-name and name-to-number mapping
functions, but IMHO it would just add extra complexity.
The dependency on the fiber compilation unit is added for key_def: it is
purely to obtain the box region in the module API functions. The key_def
compilation unit does not use anything fiber related in fact.
---
src/box/key_def.c | 141 ++++++++++++++++++
src/box/key_def.h | 157 +++++++++++++++++++-
src/exports.h | 2 +
test/app-tap/module_api.c | 240 +++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 540 insertions(+), 2 deletions(-)
diff --git a/src/box/key_def.c b/src/box/key_def.c
index a03537227..43d0c92c8 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -39,6 +39,7 @@
#include "coll_id_cache.h"
#include "small/region.h"
#include "coll/coll.h"
+#include "fiber.h"
const char *sort_order_strs[] = { "asc", "desc", "undef" };
@@ -341,6 +342,76 @@ key_def_dump_parts(const struct key_def *def, struct key_part_def *parts,
return 0;
}
+ /* {{{ Module API helpers */
+
+static int
+key_def_set_internal_part(struct key_part_def *internal_part,
+ box_key_part_def_t *part, struct region *region)
+{
+ *internal_part = key_part_def_default;
+
+ /* Set internal_part->fieldno. */
+ internal_part->fieldno = part->fieldno;
+
+ /* Set internal_part->type. */
+ if (part->field_type == NULL) {
+ diag_set(IllegalParams, "Field type is mandatory");
+ return -1;
+ }
+ size_t type_len = strlen(part->field_type);
+ internal_part->type = field_type_by_name(part->field_type, type_len);
+ if (internal_part->type == field_type_MAX) {
+ diag_set(IllegalParams, "Unknown field type: \"%s\"",
+ part->field_type);
+ return -1;
+ }
+
+ /* Set internal_part->{is_nullable,nullable_action}. */
+ bool is_nullable = (part->flags & BOX_KEY_PART_DEF_IS_NULLABLE) ==
+ BOX_KEY_PART_DEF_IS_NULLABLE;
+ if (is_nullable) {
+ internal_part->is_nullable = is_nullable;
+ internal_part->nullable_action = ON_CONFLICT_ACTION_NONE;
+ }
+
+ /* Set internal_part->coll_id. */
+ if (part->collation != NULL) {
+ size_t collation_len = strlen(part->collation);
+ struct coll_id *coll_id = coll_by_name(part->collation,
+ collation_len);
+ if (coll_id == NULL) {
+ diag_set(IllegalParams, "Unknown collation: \"%s\"",
+ part->collation);
+ return -1;
+ }
+ internal_part->coll_id = coll_id->id;
+ }
+
+ /* Set internal_part->path (JSON path). */
+ if (part->path != NULL) {
+ size_t path_len = strlen(part->path);
+ if (json_path_validate(part->path, path_len,
+ TUPLE_INDEX_BASE) != 0) {
+ diag_set(IllegalParams, "Invalid JSON path: \"%s\"",
+ part->path);
+ return -1;
+ }
+ char *tmp = region_alloc(region, path_len + 1);
+ if (tmp == NULL) {
+ diag_set(OutOfMemory, path_len + 1, "region", "path");
+ return -1;
+ }
+ memcpy(tmp, part->path, path_len + 1);
+ internal_part->path = tmp;
+ }
+
+ return 0;
+}
+
+/* }}} Module API helpers */
+
+/* {{{ Module API functions */
+
box_key_def_t *
box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
{
@@ -368,6 +439,74 @@ box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count)
return key_def;
}
+void
+box_key_part_def_create(box_key_part_def_t *part)
+{
+ memset(part, 0, sizeof(*part));
+}
+
+box_key_def_t *
+box_key_def_new_v2(box_key_part_def_t *parts, uint32_t part_count)
+{
+ if (part_count == 0) {
+ diag_set(IllegalParams, "At least one key part is required");
+ return NULL;
+ }
+
+ struct region *region = &fiber()->gc;
+ size_t region_svp = region_used(region);
+ size_t internal_parts_size;
+ struct key_part_def *internal_parts =
+ region_alloc_array(region, typeof(internal_parts[0]),
+ part_count, &internal_parts_size);
+ if (internal_parts == NULL) {
+ diag_set(OutOfMemory, internal_parts_size, "region_alloc_array",
+ "parts");
+ return NULL;
+ }
+
+ /*
+ * It is possible to implement a function similar to
+ * key_def_new() and eliminate <box_key_part_def_t> ->
+ * <struct key_part_def> copying. However this would lead
+ * to code duplication and would complicate maintanence,
+ * so it worth to do so only if key_def creation will
+ * appear on a hot path in some meaningful use case.
+ */
+ uint32_t min_field_count = 0;
+ for (uint32_t i = 0; i < part_count; ++i) {
+ if (key_def_set_internal_part(&internal_parts[i], &parts[i],
+ region) != 0) {
+ region_truncate(region, region_svp);
+ return NULL;
+ }
+ bool is_nullable =
+ (parts[i].flags & BOX_KEY_PART_DEF_IS_NULLABLE) ==
+ BOX_KEY_PART_DEF_IS_NULLABLE;
+ if (!is_nullable && parts[i].fieldno > min_field_count)
+ min_field_count = parts[i].fieldno;
+ }
+
+ struct key_def *key_def = key_def_new(internal_parts, part_count,
+ false);
+ region_truncate(region, region_svp);
+ if (key_def == NULL)
+ return NULL;
+
+ /*
+ * Update key_def->has_optional_parts and function
+ * pointers.
+ *
+ * FIXME: It seems, this call should be part of
+ * key_def_new(), because otherwise a callee function may
+ * obtain an incorrect key_def. However I don't know any
+ * case that would prove this guess.
+ */
+ key_def_update_optionality(key_def, min_field_count);
+
+ return key_def;
+}
+
void
box_key_def_delete(box_key_def_t *key_def)
{
@@ -391,6 +530,8 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
}
+/* }}} Module API functions */
+
int
key_part_cmp(const struct key_part *parts1, uint32_t part_count1,
const struct key_part *parts2, uint32_t part_count2)
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 625bb6fea..5f76890b7 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -289,14 +289,114 @@ key_def_delete(struct key_def *def);
typedef struct tuple box_tuple_t;
+/* {{{ Module API */
+
/** \cond public */
typedef struct key_def box_key_def_t;
+/** Key part definition flags. */
+enum {
+ BOX_KEY_PART_DEF_IS_NULLABLE = 1 << 0,
+};
+
+/**
+ * It is recommended to verify size of <box_key_part_def_t>
+ * against this constant on the module side at build time.
+ * Example:
+ *
+ * | #if !defined(__cplusplus) && !defined(static_assert)
+ * | #define static_assert _Static_assert
+ * | #endif
+ * |
+ * | (slash)*
+ * | * Verify that <box_key_part_def_t> has the same size when
+ * | * compiled within tarantool and within the module.
+ * | *
+ * | * It is important, because the module allocates an array of key
+ * | * parts and passes it to <box_key_def_new_v2>() tarantool
+ * | * function.
+ * | *(slash)
+ * | static_assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE,
+ * | "sizeof(box_key_part_def_t)");
+ *
+ * This snippet is not part of module.h, because portability of
+ * static_assert() / _Static_assert() is dubious. It should be
+ * decision of a module author how portable its code should be.
+ */
+enum {
+ BOX_KEY_PART_DEF_T_SIZE = 64,
+};
+
+/**
+ * Public representation of a key part definition.
+ *
+ * Usage: Allocate an array of such key parts, initialize each
+ * key part (call <box_key_part_def_create>() and set necessary
+ * fields), pass the array into <box_key_def_new_v2>() function.
+ *
+ * Important: A module should call <box_key_part_def_create>()
+ * to initialize the structure with default values. There is no
+ * guarantee that all future default values for fields and flags
+ * will be remain the same.
+ *
+ * The idea of separation from internal <struct key_part_def> is
+ * to provide stable API and ABI for modules.
+ *
+ * New fields may be added into the end of the structure in later
+ * tarantool versions. Also new flags may be introduced within
+ * <flags> field. <collation> cannot be changed to a union (to
+ * reuse for some other value), because it is verified even for
+ * a non-string key part by <box_key_def_new_v2>().
+ *
+ * Fields that are unknown at given tarantool version are ignored
+ * in general, but filled with zeros when initialized.
+ */
+typedef union PACKED {
+ struct {
+ /** Index of a tuple field (zero based). */
+ uint32_t fieldno;
+ /** Flags, e.g. nullability. */
+ uint32_t flags;
+ /** Type of the tuple field. */
+ const char *field_type;
+ /** Collation name for string comparisons. */
+ const char *collation;
+ /**
+ * JSON path to point a nested field.
+ *
+ * Example:
+ *
+ * tuple: [1, {"foo": "bar"}]
+ * key parts: [
+ * {
+ * "fieldno": 2,
+ * "type": "string",
+ * "path": "foo"
+ * }
+ * ]
+ *
+ * => key: ["bar"]
+ *
+ * Note: When the path is given, <field_type>
+ * means type of the nested field.
+ */
+ const char *path;
+ };
+ /**
+ * Padding to guarantee certain size across different
+ * tarantool versions.
+ */
+ char padding[BOX_KEY_PART_DEF_T_SIZE];
+} box_key_part_def_t;
+
/**
- * Create key definition with key fields with passed typed on passed positions.
+ * Create key definition with given field numbers and field types.
+ *
* May be used for tuple format creation and/or tuple comparison.
*
+ * \sa <box_key_def_new_v2>().
+ *
* \param fields array with key field identifiers
* \param types array with key field types (see enum field_type)
* \param part_count the number of key fields
@@ -305,6 +405,51 @@ typedef struct key_def box_key_def_t;
API_EXPORT box_key_def_t *
box_key_def_new(uint32_t *fields, uint32_t *types, uint32_t part_count);
+/**
+ * Initialize a key part with default values.
+ *
+ * | Field | Default value | Details |
+ * | ----------- | --------------- | ------- |
+ * | fieldno | 0 | |
+ * | flags | <default flags> | |
+ * | field_type | NULL | [^1] |
+ * | collation | NULL | |
+ * | path | NULL | |
+ *
+ * Default flag values are the following:
+ *
+ * | Flag | Default value |
+ * | ---------------------------- | ------------- |
+ * | BOX_KEY_PART_DEF_IS_NULLABLE | 0 (unset) |
+ *
+ * Default values of fields and flags are permitted to be changed
+ * in future tarantool versions. However we should be VERY
+ * conservative here and consider any meaningful usage scenarios,
+ * when doing so. At least new defaults should be consistent with
+ * how tarantool itself doing key_def related operations:
+ * validation, key extraction, comparisons and so on.
+ *
+ * All trailing padding bytes are set to zero. The same for
+ * unknown <flags> bits.
+ *
+ * [^1]: <box_key_def_new_v2>() does not accept NULL as a
+ * <field_type>, so it should be filled explicitly.
+ */
+API_EXPORT void
+box_key_part_def_create(box_key_part_def_t *part);
+
+/**
+ * Create a key_def from given key parts.
+ *
+ * Unlike <box_key_def_new>() this function allows to define
+ * nullability, collation and other options for each key part.
+ *
+ * <box_key_part_def_t> fields that are unknown at given tarantool
+ * version are ignored. The same for unknown <flags> bits.
+ */
+API_EXPORT box_key_def_t *
+box_key_def_new_v2(box_key_part_def_t *parts, uint32_t part_count);
+
/**
* Delete key definition
*
@@ -343,6 +488,16 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
/** \endcond public */
+/*
+ * Size of the structure should remain the same across all
+ * tarantool versions in order to allow to allocate an array of
+ * them.
+ */
+static_assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE,
+ "sizeof(box_key_part_def_t)");
+
+/* }}} Module API */
+
static inline size_t
key_def_sizeof(uint32_t part_count, uint32_t path_pool_size)
{
diff --git a/src/exports.h b/src/exports.h
index 299bb9fc1..604c1dfaa 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -31,6 +31,8 @@ EXPORT(box_iterator_free)
EXPORT(box_iterator_next)
EXPORT(box_key_def_delete)
EXPORT(box_key_def_new)
+EXPORT(box_key_def_new_v2)
+EXPORT(box_key_part_def_create)
EXPORT(box_latch_delete)
EXPORT(box_latch_lock)
EXPORT(box_latch_new)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index d7304f86d..50a7fb54e 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1,4 +1,5 @@
#include <stdbool.h>
+#include <string.h>
#include <module.h>
#include <small/ibuf.h>
@@ -319,6 +320,8 @@ error:
return 1;
}
+/* {{{ key_def api */
+
static int
test_key_def_api(lua_State *L)
{
@@ -365,6 +368,242 @@ test_key_def_api(lua_State *L)
return 1;
}
+/* }}} key_def api */
+
+/* {{{ key_def api v2 */
+
+/*
+ * More functions around key_def were exposed to the module API
+ * in order to implement external tuple.keydef and tuple.merger
+ * modules (gh-5273, gh-5384).
+ */
+
+/**
+ * Verify type and message of an error in the diagnostics area.
+ *
+ * Accepts a prefix of an actual type or a message. Pass an empty
+ * string if you need to verify only type or only message.
+ */
+static void
+check_diag(const char *exp_err_type, const char *exp_err_msg)
+{
+ box_error_t *e = box_error_last();
+ assert(strcmp(box_error_type(e), exp_err_type) == 0);
+ assert(strcmp(box_error_message(e), exp_err_msg) == 0);
+}
+
+/**
+ * Create a tuple on runtime arena.
+ *
+ * Release this tuple using <box_tuple_unref>().
+ */
+static box_tuple_t *
+new_runtime_tuple(const char *tuple_data, size_t tuple_size)
+{
+ box_tuple_format_t *fmt = box_tuple_format_default();
+ const char *tuple_end = tuple_data + tuple_size;
+ box_tuple_t *tuple = box_tuple_new(fmt, tuple_data, tuple_end);
+ assert(tuple != NULL);
+ box_tuple_ref(tuple);
+ return tuple;
+}
+
+/**
+ * Where padding bytes starts.
+ */
+static size_t
+key_part_padding_offset(void)
+{
+ if (sizeof(void *) * CHAR_BIT == 64)
+ return 32;
+ if (sizeof(void *) * CHAR_BIT == 32)
+ return 20;
+ assert(false);
+}
+
+/**
+ * Mask of all defined flags.
+ */
+static uint32_t
+key_part_def_known_flags(void)
+{
+ return BOX_KEY_PART_DEF_IS_NULLABLE;
+}
+
+/**
+ * Default flags value.
+ *
+ * All unknown bits are set to zero.
+ */
+static uint32_t
+key_part_def_default_flags(void)
+{
+ return 0;
+}
+
+/**
+ * Set all <box_key_part_def_t> fields to nondefault values.
+ *
+ * It also set all padding bytes and unknown flags to non-zero
+ * values.
+ */
+static void
+key_part_def_set_nondefault(box_key_part_def_t *part)
+{
+ size_t padding_offset = key_part_padding_offset();
+ uint32_t default_flags = key_part_def_default_flags();
+
+ /*
+ * Give correct non-default values for known fields and
+ * flags. Set unknown flags to non-zero values.
+ */
+ part->fieldno = 1;
+ part->flags = ~default_flags;
+ part->field_type = "string";
+ part->collation = "unicode_ci";
+ part->path = "foo";
+
+ /* Fill padding with non-zero bytes. */
+ char *padding = ((char *) part) + padding_offset;
+ size_t padding_size = sizeof(box_key_part_def_t) - padding_offset;
+ memset(padding, 0xff, padding_size);
+}
+
+/**
+ * Verify that all known fields and flags are set to default
+ * values.
+ */
+static void
+key_part_def_check_default(box_key_part_def_t *part)
+{
+ uint32_t known_flags = key_part_def_known_flags();
+ uint32_t default_flags = key_part_def_default_flags();
+
+ assert(part->fieldno == 0);
+ assert((part->flags & known_flags) == default_flags);
+ assert(part->field_type == NULL);
+ assert(part->collation == NULL);
+ assert(part->path == NULL);
+}
+
+/**
+ * Verify that all padding bytes and unknown flags are set to
+ * zeros.
+ */
+static void
+key_part_def_check_zeros(const box_key_part_def_t *part)
+{
+ size_t padding_offset = key_part_padding_offset();
+ uint32_t unknown_flags = ~key_part_def_known_flags();
+
+ char *padding = ((char *) part) + padding_offset;
+ char *padding_end = ((char *) part) + sizeof(box_key_part_def_t);
+ for (char *p = padding; p < padding_end; ++p) {
+ assert(*p == 0);
+ }
+
+ assert((part->flags & unknown_flags) == 0);
+}
+
+/**
+ * Basic <box_key_part_def_create>() and <box_key_def_new_v2>()
+ * test.
+ */
+static int
+test_key_def_new_v2(struct lua_State *L)
+{
+ /* Verify <box_key_part_def_t> binary layout. */
+ assert(BOX_KEY_PART_DEF_T_SIZE == 64);
+ assert(sizeof(box_key_part_def_t) == BOX_KEY_PART_DEF_T_SIZE);
+ assert(offsetof(box_key_part_def_t, fieldno) == 0);
+ assert(offsetof(box_key_part_def_t, flags) == 4);
+ assert(offsetof(box_key_part_def_t, field_type) == 8);
+ if (sizeof(void *) * CHAR_BIT == 64) {
+ assert(offsetof(box_key_part_def_t, collation) == 16);
+ assert(offsetof(box_key_part_def_t, path) == 24);
+ } else if (sizeof(void *) * CHAR_BIT == 32) {
+ assert(offsetof(box_key_part_def_t, collation) == 12);
+ assert(offsetof(box_key_part_def_t, path) == 16);
+ } else {
+ assert(false);
+ }
+
+ /*
+ * Fill key part definition with nondefault values.
+ * Fill padding and unknown flags with non-zero values.
+ */
+ box_key_part_def_t part;
+ key_part_def_set_nondefault(&part);
+
+ /*
+ * Verify that all known fields are set to default values and
+ * all unknown fields and flags are set to zeros.
+ */
+ box_key_part_def_create(&part);
+ key_part_def_check_default(&part);
+ key_part_def_check_zeros(&part);
+
+ box_key_def_t *key_def;
+
+ /* Should not accept zero part count. */
+ key_def = box_key_def_new_v2(NULL, 0);
+ assert(key_def == NULL);
+ check_diag("IllegalParams", "At least one key part is required");
+
+ /* Should not accept NULL as a <field_type>. */
+ key_def = box_key_def_new_v2(&part, 1);
+ assert(key_def == NULL);
+ check_diag("IllegalParams", "Field type is mandatory");
+
+ /* Success case. */
+ part.field_type = "unsigned";
+ key_def = box_key_def_new_v2(&part, 1);
+ assert(key_def != NULL);
+
+ /*
+ * Prepare tuples to do some comparisons.
+ *
+ * [1, 2, 3] and [3, 2, 1].
+ */
+ box_tuple_t *tuple_1 = new_runtime_tuple("\x93\x01\x02\x03", 4);
+ box_tuple_t *tuple_2 = new_runtime_tuple("\x93\x03\x02\x01", 4);
+
+ /*
+ * Verify that key_def actually can be used in functions
+ * that accepts it.
+ *
+ * Do several comparisons. Far away from being an
+ * exhaustive comparator test.
+ */
+ int rc;
+ rc = box_tuple_compare(tuple_1, tuple_1, key_def);
+ assert(rc == 0);
+ rc = box_tuple_compare(tuple_2, tuple_2, key_def);
+ assert(rc == 0);
+ rc = box_tuple_compare(tuple_1, tuple_2, key_def);
+ assert(rc < 0);
+ rc = box_tuple_compare(tuple_2, tuple_1, key_def);
+ assert(rc > 0);
+
+ /* The same idea, but perform comparisons against keys. */
+ rc = box_tuple_compare_with_key(tuple_1, "\x91\x00", key_def);
+ assert(rc > 0);
+ rc = box_tuple_compare_with_key(tuple_1, "\x91\x01", key_def);
+ assert(rc == 0);
+ rc = box_tuple_compare_with_key(tuple_1, "\x91\x02", key_def);
+ assert(rc < 0);
+
+ /* Clean up. */
+ box_tuple_unref(tuple_1);
+ box_tuple_unref(tuple_2);
+ box_key_def_delete(key_def);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
+/* }}} key_def api v2 */
+
static int
check_error(lua_State *L)
{
@@ -740,6 +979,7 @@ luaopen_module_api(lua_State *L)
{"test_box_region", test_box_region},
{"test_tuple_encode", test_tuple_encode},
{"test_tuple_new", test_tuple_new},
+ {"test_key_def_new_v2", test_key_def_new_v2},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index f1b377e17..5145ab164 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(28)
+ test:plan(29)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 11/16] module api: add box_key_def_dump_parts()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (9 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 10/16] module api: add box_key_def_new_v2() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 12/16] module api: expose box_key_def_validate_tuple() Alexander Turenko
` (7 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
The function dumps an opaque <box_key_def_t> structure into a non-opaque
array of <box_key_part_def_t> structures in order to allow an external
module to obtain information about the key definition.
Part of #5273
---
src/box/key_def.c | 78 ++++++++++++++++++
src/box/key_def.h | 13 +++
src/exports.h | 1 +
test/app-tap/module_api.c | 135 +++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 228 insertions(+), 1 deletion(-)
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 43d0c92c8..3c2cf1d7f 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -513,6 +513,84 @@ box_key_def_delete(box_key_def_t *key_def)
key_def_delete(key_def);
}
+box_key_part_def_t *
+box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr)
+{
+ struct region *region = &fiber()->gc;
+ size_t region_svp = region_used(region);
+ size_t size;
+ box_key_part_def_t *parts = region_alloc_array(
+ region, typeof(parts[0]), key_def->part_count, &size);
+ if (parts == NULL) {
+ diag_set(OutOfMemory, size, "region_alloc_array", "parts");
+ return NULL;
+ }
+
+ for (uint32_t i = 0; i < key_def->part_count; i++) {
+ const struct key_part *part = &key_def->parts[i];
+ box_key_part_def_t *part_def = &parts[i];
+ box_key_part_def_create(part_def);
+
+ /* Set part->{fieldno,flags,field_type}. */
+ part_def->fieldno = part->fieldno;
+ part_def->flags = 0;
+ if (key_part_is_nullable(part))
+ part_def->flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+ assert(part->type >= 0 && part->type < field_type_MAX);
+ part_def->field_type = field_type_strs[part->type];
+
+ /* Set part->collation. */
+ if (part->coll_id != COLL_NONE) {
+ struct coll_id *coll_id = coll_by_id(part->coll_id);
+ /*
+ * A collation may be removed after
+ * key_def creation.
+ */
+ if (coll_id == NULL) {
+ diag_set(CollationError,
+ "key_def holds dead collation id %d",
+ part->coll_id);
+ region_truncate(region, region_svp);
+ return NULL;
+ }
+ /*
+ * A collation may be removed while the
+ * resulting key parts array is in use.
+ */
+ char *collation = region_alloc(region,
+ coll_id->name_len + 1);
+ if (collation == NULL) {
+ diag_set(OutOfMemory, coll_id->name_len + 1,
+ "region_alloc", "part_def->collation");
+ region_truncate(region, region_svp);
+ return NULL;
+ }
+ memcpy(collation, coll_id->name, coll_id->name_len);
+ collation[coll_id->name_len] = '\0';
+ part_def->collation = collation;
+ }
+
+ /* Set part->path. */
+ if (part->path != NULL) {
+ char *path = region_alloc(region, part->path_len + 1);
+ if (path == NULL) {
+ diag_set(OutOfMemory, part->path_len + 1,
+ "region", "part_def->path");
+ region_truncate(region, region_svp);
+ return NULL;
+ }
+ memcpy(path, part->path, part->path_len);
+ path[part->path_len] = '\0';
+ part_def->path = path;
+ }
+ }
+
+ if (part_count_ptr != NULL)
+ *part_count_ptr = key_def->part_count;
+
+ return parts;
+}
+
int
box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
box_key_def_t *key_def)
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 5f76890b7..3e5922302 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -458,6 +458,19 @@ box_key_def_new_v2(box_key_part_def_t *parts, uint32_t part_count);
API_EXPORT void
box_key_def_delete(box_key_def_t *key_def);
+/**
+ * Dump key part definitions of given key_def.
+ *
+ * The function allocates key parts and storage for pointer fields
+ * (e.g. collation names) on the box region.
+ * @sa <box_region_truncate>().
+ *
+ * <box_key_part_def_t> fields that are unknown at given tarantool
+ * version are set to zero. The same for unknown <flags> bits.
+ */
+API_EXPORT box_key_part_def_t *
+box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr);
+
/**
* Compare tuples using the key definition.
* @param tuple_a first tuple
diff --git a/src/exports.h b/src/exports.h
index 604c1dfaa..8ffc4d887 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -30,6 +30,7 @@ EXPORT(box_insert)
EXPORT(box_iterator_free)
EXPORT(box_iterator_next)
EXPORT(box_key_def_delete)
+EXPORT(box_key_def_dump_parts)
EXPORT(box_key_def_new)
EXPORT(box_key_def_new_v2)
EXPORT(box_key_part_def_create)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 50a7fb54e..e3f127b5e 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -378,6 +378,22 @@ test_key_def_api(lua_State *L)
* modules (gh-5273, gh-5384).
*/
+/**
+ * Verify that two zero terminated strings are either both NULL
+ * or have equal values.
+ */
+static void
+string_check_equal(const char *a, const char *b)
+{
+ if (a == NULL) {
+ assert(b == NULL);
+ } else {
+ assert(b != NULL);
+ assert(strlen(a) == strlen(b));
+ assert(strcmp(a, b) == 0);
+ }
+}
+
/**
* Verify type and message of an error in the diagnostics area.
*
@@ -505,6 +521,25 @@ key_part_def_check_zeros(const box_key_part_def_t *part)
assert((part->flags & unknown_flags) == 0);
}
+/**
+ * Check that two key part definitions are equal.
+ *
+ * It compares only known fields and flags, but ignores padding
+ * bytes and unknown flags.
+ */
+static void
+key_part_def_check_equal(const box_key_part_def_t *a,
+ const box_key_part_def_t *b)
+{
+ uint32_t known_flags = key_part_def_known_flags();
+
+ assert(a->fieldno == b->fieldno);
+ assert((a->flags & known_flags) == (b->flags & known_flags));
+ string_check_equal(a->field_type, b->field_type);
+ string_check_equal(a->collation, b->collation);
+ string_check_equal(a->path, b->path);
+}
+
/**
* Basic <box_key_part_def_create>() and <box_key_def_new_v2>()
* test.
@@ -602,6 +637,105 @@ test_key_def_new_v2(struct lua_State *L)
return 1;
}
+/**
+ * Basic <box_key_def_dump_parts>() test.
+ */
+static int
+test_key_def_dump_parts(struct lua_State *L)
+{
+ size_t region_svp = box_region_used();
+ box_key_def_t *key_def = NULL;
+ box_key_part_def_t *dump = NULL;
+ uint32_t dump_part_count = 0;
+
+ /*
+ * Create a key_def with a single key part with all fields
+ * and flags set to non-default values.
+ */
+ box_key_part_def_t part;
+ key_part_def_set_nondefault(&part);
+ key_def = box_key_def_new_v2(&part, 1);
+ assert(key_def != NULL);
+
+ /*
+ * Verify that the same values are dumped, but unknown
+ * fields and flags are set to zeros.
+ */
+ dump = box_key_def_dump_parts(key_def, &dump_part_count);
+ assert(dump != NULL);
+ assert(dump_part_count == 1);
+ key_part_def_check_equal(&part, &dump[0]);
+ key_part_def_check_zeros(&dump[0]);
+
+ /* We can pass NULL as <part_count_ptr>. */
+ dump = box_key_def_dump_parts(key_def, NULL);
+ assert(dump != NULL);
+
+ /* Clean up. */
+ box_key_def_delete(key_def);
+
+ /* Create a key_def from two key part definitions. */
+ box_key_part_def_t parts[2];
+ box_key_part_def_create(&parts[0]);
+ box_key_part_def_create(&parts[1]);
+ parts[0].fieldno = 19;
+ parts[0].field_type = "unsigned";
+ parts[0].path = "foo";
+ parts[1].fieldno = 7;
+ parts[1].field_type = "string";
+ parts[1].collation = "unicode";
+ parts[1].flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+ key_def = box_key_def_new_v2(parts, 2);
+ assert(key_def != NULL);
+
+ /* Verify how it'll be dumped. */
+ dump = box_key_def_dump_parts(key_def, &dump_part_count);
+ assert(dump != NULL);
+ assert(dump_part_count == 2);
+ key_part_def_check_equal(&parts[0], &dump[0]);
+ key_part_def_check_equal(&parts[1], &dump[1]);
+
+ /* Clean up. */
+ box_key_def_delete(key_def);
+
+ /* Can we again create a key_def from the dumped parts? */
+ key_def = box_key_def_new_v2(dump, dump_part_count);
+ assert(key_def != NULL);
+
+ /* Verify this dump based key_def. */
+ dump = box_key_def_dump_parts(key_def, &dump_part_count);
+ assert(dump != NULL);
+ assert(dump_part_count == 2);
+ key_part_def_check_equal(&parts[0], &dump[0]);
+ key_part_def_check_equal(&parts[1], &dump[1]);
+
+ /* Clean up. */
+ box_key_def_delete(key_def);
+
+ /*
+ * 'none' collation is the same as lack of a collation
+ * from key_def point of view. In the dump it is present
+ * as NULL.
+ */
+ parts[1].collation = "none";
+ key_def = box_key_def_new_v2(parts, 2);
+ assert(key_def != NULL);
+ dump = box_key_def_dump_parts(key_def, &dump_part_count);
+ assert(dump != NULL);
+ assert(dump_part_count == 2);
+ /* Set to NULL just for ease verification. */
+ parts[1].collation = NULL;
+ key_part_def_check_equal(&parts[0], &dump[0]);
+ key_part_def_check_equal(&parts[1], &dump[1]);
+
+ /* Clean up. */
+ box_key_def_delete(key_def);
+ box_region_truncate(region_svp);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
/* }}} key_def api v2 */
static int
@@ -980,6 +1114,7 @@ luaopen_module_api(lua_State *L)
{"test_tuple_encode", test_tuple_encode},
{"test_tuple_new", test_tuple_new},
{"test_key_def_new_v2", test_key_def_new_v2},
+ {"test_key_def_dump_parts", test_key_def_dump_parts},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 5145ab164..f4edfad10 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(29)
+ test:plan(30)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 12/16] module api: expose box_key_def_validate_tuple()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (10 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 11/16] module api: add box_key_def_dump_parts() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 13/16] module api: expose box_key_def_merge() Alexander Turenko
` (6 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
Part of #5273
---
src/box/key_def.c | 6 ++
src/box/key_def.h | 12 ++++
src/exports.h | 1 +
test/app-tap/module_api.c | 94 ++++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 114 insertions(+), 1 deletion(-)
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 3c2cf1d7f..7f134aa98 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -591,6 +591,12 @@ box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr)
return parts;
}
+int
+box_key_def_validate_tuple(box_key_def_t *key_def, box_tuple_t *tuple)
+{
+ return tuple_validate_key_parts(key_def, tuple);
+}
+
int
box_tuple_compare(box_tuple_t *tuple_a, box_tuple_t *tuple_b,
box_key_def_t *key_def)
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 3e5922302..304deedff 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -471,6 +471,18 @@ box_key_def_delete(box_key_def_t *key_def);
API_EXPORT box_key_part_def_t *
box_key_def_dump_parts(const box_key_def_t *key_def, uint32_t *part_count_ptr);
+/**
+ * Check that tuple fields match with given key definition.
+ *
+ * @param key_def Key definition.
+ * @param tuple Tuple to validate.
+ *
+ * @retval 0 The tuple is valid.
+ * @retval -1 The tuple is invalid.
+ */
+API_EXPORT int
+box_key_def_validate_tuple(box_key_def_t *key_def, box_tuple_t *tuple);
+
/**
* Compare tuples using the key definition.
* @param tuple_a first tuple
diff --git a/src/exports.h b/src/exports.h
index 8ffc4d887..71ec1a9b5 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -33,6 +33,7 @@ EXPORT(box_key_def_delete)
EXPORT(box_key_def_dump_parts)
EXPORT(box_key_def_new)
EXPORT(box_key_def_new_v2)
+EXPORT(box_key_def_validate_tuple)
EXPORT(box_key_part_def_create)
EXPORT(box_latch_delete)
EXPORT(box_latch_lock)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index e3f127b5e..2f1ceefb6 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -736,6 +736,99 @@ test_key_def_dump_parts(struct lua_State *L)
return 1;
}
+/**
+ * Basic <box_key_def_validate_tuple>() test.
+ */
+static int
+test_key_def_validate_tuple(struct lua_State *L)
+{
+ /*
+ * Create a key_def.
+ *
+ * | tuple
+ * | [x, x, x]
+ * | key_def ^ ^
+ * | | | |
+ * | (0) <-----+---- string (optional)
+ * | | |
+ * | (1) <---- unsigned
+ */
+ box_key_part_def_t parts[2];
+ box_key_part_def_create(&parts[0]);
+ box_key_part_def_create(&parts[1]);
+ parts[0].fieldno = 2;
+ parts[0].field_type = "string";
+ parts[0].flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+ parts[1].fieldno = 0;
+ parts[1].field_type = "unsigned";
+ box_key_def_t *key_def = box_key_def_new_v2(parts, 2);
+ assert(key_def != NULL);
+
+ /*
+ * Create tuples to validate them against given key_def.
+ *
+ * | # | tuple | Is valid? |
+ * | - | ------------- | --------- |
+ * | 0 | [1, 2, "moo"] | valid |
+ * | 1 | [1, 2, null] | valid |
+ * | 2 | [1, 2] | valid |
+ * | 3 | [1] | valid |
+ * | 4 | [] | invalid |
+ * | 5 | [1, 2, 3] | invalid |
+ * | 6 | ["moo"] | invalid |
+ * | 7 | [-1] | invalid |
+ */
+ box_tuple_t *tuples[] = {
+ /* [0] = */ new_runtime_tuple("\x93\x01\x02\xa3moo", 7),
+ /* [1] = */ new_runtime_tuple("\x93\x01\x02\xc0", 4),
+ /* [2] = */ new_runtime_tuple("\x92\x01\x02", 3),
+ /* [3] = */ new_runtime_tuple("\x91\x01", 2),
+ /* [4] = */ new_runtime_tuple("\x90", 1),
+ /* [5] = */ new_runtime_tuple("\x93\x01\x02\x03", 4),
+ /* [6] = */ new_runtime_tuple("\x91\xa3moo", 5),
+ /* [7] = */ new_runtime_tuple("\x91\xff", 2),
+ };
+ int expected_results[] = {
+ /* [0] = */ 0,
+ /* [1] = */ 0,
+ /* [2] = */ 0,
+ /* [3] = */ 0,
+ /* [4] = */ -1,
+ /* [5] = */ -1,
+ /* [6] = */ -1,
+ /* [7] = */ -1,
+ };
+ uint32_t expected_error_codes[] = {
+ /* [0] = */ box_error_code_MAX,
+ /* [1] = */ box_error_code_MAX,
+ /* [2] = */ box_error_code_MAX,
+ /* [3] = */ box_error_code_MAX,
+ /* [4] = */ ER_FIELD_MISSING,
+ /* [5] = */ ER_KEY_PART_TYPE,
+ /* [6] = */ ER_KEY_PART_TYPE,
+ /* [7] = */ ER_KEY_PART_TYPE,
+ };
+
+ for (size_t i = 0; i < lengthof(tuples); ++i) {
+ int rc = box_key_def_validate_tuple(key_def, tuples[i]);
+ assert(rc == expected_results[i]);
+
+ if (expected_error_codes[i] != box_error_code_MAX) {
+ assert(rc != 0);
+ box_error_t *e = box_error_last();
+ assert(box_error_code(e) == expected_error_codes[i]);
+ }
+ }
+
+ /* Clean up. */
+ for (size_t i = 0; i < lengthof(tuples); ++i)
+ box_tuple_unref(tuples[i]);
+ box_key_def_delete(key_def);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
/* }}} key_def api v2 */
static int
@@ -1115,6 +1208,7 @@ luaopen_module_api(lua_State *L)
{"test_tuple_new", test_tuple_new},
{"test_key_def_new_v2", test_key_def_new_v2},
{"test_key_def_dump_parts", test_key_def_dump_parts},
+ {"test_key_def_validate_tuple", test_key_def_validate_tuple},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index f4edfad10..dbea9343b 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(30)
+ test:plan(31)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 13/16] module api: expose box_key_def_merge()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (11 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 12/16] module api: expose box_key_def_validate_tuple() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 14/16] module api: expose box_key_def_extract_key() Alexander Turenko
` (5 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
Part of #5273
---
src/box/key_def.c | 6 +
src/box/key_def.h | 14 +
src/exports.h | 1 +
test/app-tap/module_api.c | 752 +++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 774 insertions(+), 1 deletion(-)
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 7f134aa98..7226f2482 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -614,6 +614,12 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
}
+box_key_def_t *
+box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
+{
+ return key_def_merge(first, second);
+}
+
/* }}} Module API functions */
int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 304deedff..fdf65dec6 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -511,6 +511,20 @@ API_EXPORT int
box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
box_key_def_t *key_def);
+/**
+ * Allocate a new key_def with a set union of key parts from
+ * first and second key defs.
+ *
+ * Parts of the new key_def consist of the first key_def's parts
+ * and those parts of the second key_def that were not among the
+ * first parts.
+ *
+ * @retval not NULL Ok.
+ * @retval NULL Memory error.
+ */
+API_EXPORT box_key_def_t *
+box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second);
+
/** \endcond public */
/*
diff --git a/src/exports.h b/src/exports.h
index 71ec1a9b5..223390d52 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -31,6 +31,7 @@ EXPORT(box_iterator_free)
EXPORT(box_iterator_next)
EXPORT(box_key_def_delete)
EXPORT(box_key_def_dump_parts)
+EXPORT(box_key_def_merge)
EXPORT(box_key_def_new)
EXPORT(box_key_def_new_v2)
EXPORT(box_key_def_validate_tuple)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 2f1ceefb6..175217ef9 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -540,6 +540,38 @@ key_part_def_check_equal(const box_key_part_def_t *a,
string_check_equal(a->path, b->path);
}
+/**
+ * Check <box_key_def_merge>() result against expected one.
+ *
+ * Allocates temporary values on the box region (a caller should
+ * release them).
+ */
+static void
+key_def_check_merge(box_key_part_def_t *a, uint32_t part_count_a,
+ box_key_part_def_t *b, uint32_t part_count_b,
+ box_key_part_def_t *exp, uint32_t part_count_exp)
+{
+ box_key_def_t *key_def_a = box_key_def_new_v2(a, part_count_a);
+ assert(key_def_a != NULL);
+ box_key_def_t *key_def_b = box_key_def_new_v2(b, part_count_b);
+ assert(key_def_b != NULL);
+
+ box_key_def_t *key_def_res = box_key_def_merge(key_def_a, key_def_b);
+ uint32_t part_count_res;
+ box_key_part_def_t *res = box_key_def_dump_parts(key_def_res,
+ &part_count_res);
+ assert(res != NULL);
+
+ assert(part_count_res == part_count_exp);
+ for (uint32_t i = 0; i < part_count_exp; ++i) {
+ key_part_def_check_equal(&res[i], &exp[i]);
+ }
+
+ box_key_def_delete(key_def_res);
+ box_key_def_delete(key_def_b);
+ box_key_def_delete(key_def_a);
+}
+
/**
* Basic <box_key_part_def_create>() and <box_key_def_new_v2>()
* test.
@@ -829,6 +861,725 @@ test_key_def_validate_tuple(struct lua_State *L)
return 1;
}
+/**
+ * Basic <box_key_def_merge>() test.
+ */
+static int
+test_key_def_merge(struct lua_State *L)
+{
+ /*
+ * What is the idea of <box_key_def_merge>()?
+ *
+ * (In my humble understanding.)
+ *
+ * For any given kd1 and kd2, kd3 = merge(kd1, kd2) should
+ * impose the same order of tuples as if they would be
+ * ordered by kd1, but all kd1-equal tuples would be
+ * ordered by kd2.
+ *
+ * We could just add all key parts of kd2 to kd1 parts.
+ * However in some cases we can skip some of kd2 parts
+ * (the simplest case: when they are equal). That is what
+ * <box_key_def_merge>() doing in fact.
+ *
+ * Should we provide a guarantee that first len(kd1) parts
+ * of kd3 = merge(kd1, kd2) will be the same as in kd1? Or
+ * those key parts can be strengthen with turning off
+ * nullability, picking up more restrictive field type or
+ * choosing of a more restrictive collation if such
+ * restrictions are defined by kd2?
+ *
+ * The tuples ordering property is guaranteed by the
+ * implementation. In particular, it leans on the fact
+ * that a comparator for a more general type imposes the
+ * same ordering on a more restrictive type as if when a
+ * type-specific comparator is be used. E.g. an order of
+ * any two given unsigned integers is the same when we
+ * comparing them as unsigned integers, as integers, as
+ * numbers or as scalars (note: we don't have comparators
+ * for 'any' type).
+ *
+ * However <box_key_def_t> provides not only comparator
+ * functions, but also validation and key extraction ones.
+ *
+ * Let's consider validation. It looks logical to expect
+ * that the following invariant is guaranteed: for any
+ * given kd1 and kd2, kd3 = merge(kd1, kd2) should accept
+ * only those tuples that both kd1 and kd2 accept (kd
+ * accepts a tuple when it is valid against kd). This is
+ * not so now.
+ *
+ * If the function would impose this guarantee, it must
+ * pay attention to field types compatibility (and which
+ * ones are more restrictive than others) and nullability.
+ * Not sure whether a collation may restrict a set of
+ * possible values (in theory it may be so; at least not
+ * any byte sequence forms a valid UTF-8 string).
+ *
+ * It also looks logical to expect that, when sets of
+ * tuples that are accepted by kd1 and that are accepted
+ * by kd2 have the empty intersection, the merge function
+ * will give an error. It is not so now too.
+ *
+ * If the function would impose this guarantee, it must
+ * handle the case, when the same field is marked with
+ * incompatible types and both key part definitions are
+ * non-nullable. Not sure that it is the only point that
+ * must be taken into account here.
+ *
+ * Now let's consider key extraction from a tuple. For
+ * given kd1 and kd2, a change of the merge algorithm may
+ * change parts count in kd3 = merge(kd1, kd2) and so
+ * parts count in a key extracted by it. It is hard to
+ * say, which guarantees we should provide here. So,
+ * maybe, if we'll touch the merge algorithm, we should
+ * leave the old function as is and expose _v2() function.
+ *
+ * On the other hand, having two implementations of the
+ * merge function with different guarantees, where only
+ * the older one will be used internally is somewhat
+ * strange and may lead to sudden inconsistencies.
+ *
+ * If we'll look at the <box_key_def_merge>() from the
+ * practical point of view, the only known usage of this
+ * function is to provide a comparator that gives exactly
+ * same order as a secondary index in tarantool (when it
+ * is not unique, secondary key parts are merged with the
+ * primary ones). So, it seems, if something should be
+ * changed, it should be changed in sync with internals.
+ *
+ * To sum up: current behaviour is the controversial topic
+ * and we may want to reconsider it in some way in a
+ * future. So let's look to some of the test cases below
+ * as on examples of current behaviour: not as on a
+ * commitment that it'll be the same forever (while the
+ * main property regarding tuples ordering is hold).
+ */
+
+ size_t region_svp = box_region_used();
+
+ /*
+ * Causion: Don't initialize <box_key_part_def_t> directly
+ * in a real world code. Use <box_key_part_def_create>().
+ *
+ * The testing code is updated in sync with tarantool, so
+ * it may lean on the knowledge about particular set of
+ * fields and flags.
+ *
+ * In contrast a module should be able to be built against
+ * an older version of tarantool and correctly run on a
+ * newer one. It also should be able to build against the
+ * newer tarantool version without code changes.
+ *
+ * The <box_key_part_def_t> structure may be updated in a
+ * future version of tarantool. The only permitted updates
+ * are adding new fields or flags, or update of a default
+ * value of a field or a flag. Let's show how it may break
+ * non-conventional code:
+ *
+ * 1. Case: a new field is added.
+ *
+ * As result, if brace initializer is used,
+ * -Wmissing-field-initializers (part of -Wextra)
+ * warning may be produced when building a module
+ * against the new tarantool version. Usage of -Werror
+ * for the Debug build is usual, so it may break
+ * compilation.
+ *
+ * 2. Case: a new field or flag is added with non-zero
+ * default value or a default value of some field or
+ * flag is changed.
+ *
+ * As result a module will initialize the new / changed
+ * fields or flags with values that are not default for
+ * given tarantool version, but may assume that
+ * everything that is not set explicitly is default.
+ */
+
+ /* Non-conventional prerequisite: no new fields. */
+ size_t padding_offset = key_part_padding_offset();
+ size_t path_field_end = offsetof(box_key_part_def_t, path) +
+ sizeof(const char *);
+ assert(padding_offset == path_field_end);
+
+ /* Non-conventional prerequisite: list of known flags. */
+ uint32_t known_flags = key_part_def_known_flags();
+ assert(known_flags == BOX_KEY_PART_DEF_IS_NULLABLE);
+
+ /* Non-conventional prerequisite: certain defaults. */
+ box_key_part_def_t tmp;
+ box_key_part_def_create(&tmp);
+ assert((tmp.flags & BOX_KEY_PART_DEF_IS_NULLABLE) == 0);
+ assert(tmp.collation == NULL);
+ assert(tmp.path == NULL);
+
+ /*
+ * The extra parentheses are necessary to initialize
+ * <box_key_part_def_t>, because it is a union around an
+ * anonymous structure and padding, not a structure.
+ */
+
+ /* Case 1: all <fieldno> are different. */
+ box_key_part_def_t a_1[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t b_1[] = {
+ {{0, 0, "unsigned", NULL, NULL}},
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_1[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}},
+ {{0, 0, "unsigned", NULL, NULL}},
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_1, lengthof(a_1), b_1, lengthof(b_1),
+ exp_1, lengthof(exp_1));
+
+ /* Case 2: two key parts are the same. */
+ box_key_part_def_t a_2[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* clash */
+ };
+ box_key_part_def_t b_2[] = {
+ {{1, 0, "unsigned", NULL, NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_2[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_2, lengthof(a_2), b_2, lengthof(b_2),
+ exp_2, lengthof(exp_2));
+
+ /*
+ * Case 3: more general field type + more restrictive one.
+ *
+ * Interpretation: when <a> and <b> have key parts that
+ * are point to the same field (considering <fieldno> and
+ * JSON paths) and collations are not present or don't
+ * impose any restrictions, the key part from <b> is
+ * omitted without any care to <field_type> and <flags>.
+ */
+ box_key_part_def_t a_3[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "number", NULL, NULL}}, /* clash */
+ };
+ box_key_part_def_t b_3[] = {
+ {{1, 0, "unsigned", NULL, NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_3[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "number", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_3, lengthof(a_3), b_3, lengthof(b_3),
+ exp_3, lengthof(exp_3));
+
+ /*
+ * Case 4: more restrictive field type + more general one.
+ *
+ * Interpretation: the same as for the case 3.
+ */
+ box_key_part_def_t a_4[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* clash */
+ };
+ box_key_part_def_t b_4[] = {
+ {{1, 0, "number", NULL, NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_4[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_4, lengthof(a_4), b_4, lengthof(b_4),
+ exp_4, lengthof(exp_4));
+
+ /*
+ * Case 5: incompatible field types.
+ *
+ * Interpretation: the same as for the case 3.
+ */
+ box_key_part_def_t a_5[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* clash */
+ };
+ box_key_part_def_t b_5[] = {
+ {{1, 0, "string", NULL, NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_5[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_5, lengthof(a_5), b_5, lengthof(b_5),
+ exp_5, lengthof(exp_5));
+
+ /*
+ * Case 6: nullable + non-nullable.
+ *
+ * Interpretation: the same as for the case 3.
+ */
+ box_key_part_def_t a_6[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 1, "unsigned", NULL, NULL}}, /* clash */
+ };
+ box_key_part_def_t b_6[] = {
+ {{1, 0, "unsigned", NULL, NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_6[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 1, "unsigned", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_6, lengthof(a_6), b_6, lengthof(b_6),
+ exp_6, lengthof(exp_6));
+
+ /*
+ * Case 7: non-nullable + nullable.
+ *
+ * Interpretation: the same as for the case 3.
+ */
+ box_key_part_def_t a_7[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* clash */
+ };
+ box_key_part_def_t b_7[] = {
+ {{1, 1, "unsigned", NULL, NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_7[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "unsigned", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_7, lengthof(a_7), b_7, lengthof(b_7),
+ exp_7, lengthof(exp_7));
+
+ /*
+ * Case 8: the same ICU collations.
+ *
+ * Interpretation: when <a> and <b> have key parts that
+ * are point to the same field (considering <fieldno> and
+ * JSON paths), the key part from <b> is omitted, when one
+ * of the following conditions is true:
+ *
+ * 1. <a> and <b> have the same collation (or both lacks
+ * it).
+ * 2. <a> has no collation.
+ * 3. <a> has a non-ICU collation (those are 'none' and
+ * 'binary' now).
+ * 4. <a> has an ICU collation with UCOL_DEFAULT strength
+ * (but I don't know what does it mean in practice and
+ * unable to interpret).
+ *
+ * Comments around <coll_can_merge>() point the general
+ * idea: don't coalesce when <b>'s collation may impose
+ * a strict order on keys equal in terms of the <a>'s
+ * collation. (And I guess 'more strict' was meant by the
+ * word 'strict'.)
+ *
+ * The general rule is to don't coalesce when doubt. But
+ * under the conditions above we're sure that the order
+ * imposed by <a>'s collation is already strict and hence
+ * we don't need <b>'s collation at all.
+ *
+ * Beware! Tarantool-1.10 does not take collations into
+ * account at all when decide whether to coalesce a key
+ * part or not. See gh-3537.
+ *
+ * Aside of this, tarantool-1.10 only have 'unicode' and
+ * 'unicode_ci' collations.
+ */
+ box_key_part_def_t a_8[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_8[] = {
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_8[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_8, lengthof(a_8), b_8, lengthof(b_8),
+ exp_8, lengthof(exp_8));
+
+ /*
+ * Case 9: no collation + ICU collation.
+ *
+ * Interpretation: see the case 8.
+ */
+ box_key_part_def_t a_9[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", NULL, NULL}}, /* clash */
+ };
+ box_key_part_def_t b_9[] = {
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_9[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_9, lengthof(a_9), b_9, lengthof(b_9),
+ exp_9, lengthof(exp_9));
+
+ /*
+ * Case 10: ICU collation + no collation.
+ *
+ * Interpretation: see the case 8.
+ */
+ box_key_part_def_t a_10[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_10[] = {
+ {{1, 0, "string", NULL, NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_10[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* from <a> */
+ {{1, 0, "string", NULL, NULL}}, /* from <b> */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_10, lengthof(a_10), b_10, lengthof(b_10),
+ exp_10, lengthof(exp_10));
+
+ /*
+ * Case 11: less strong ICU collation + more strong one,
+ * but with the same locale.
+ *
+ * 'Less strong' means 'have smaller strength' here.
+ *
+ * Interpretation: see the case 8.
+ */
+ box_key_part_def_t a_11[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode_ci", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_11[] = {
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_11[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode_ci", NULL}}, /* from <a> */
+ {{1, 0, "string", "unicode", NULL}}, /* from <b> */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_11, lengthof(a_11), b_11, lengthof(b_11),
+ exp_11, lengthof(exp_11));
+
+ /*
+ * Case 12: more strong ICU collation + less strong one,
+ * but with the same locale.
+ *
+ * 'More strong' means 'have bigger strength' here.
+ *
+ * Interpretation: see the case 8.
+ */
+ box_key_part_def_t a_12[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_12[] = {
+ {{1, 0, "string", "unicode_ci", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_12[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* from <a> */
+ {{1, 0, "string", "unicode_ci", NULL}}, /* from <b> */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_12, lengthof(a_12), b_12, lengthof(b_12),
+ exp_12, lengthof(exp_12));
+
+ /*
+ * Case 13: ICU collations with different locales.
+ *
+ * Interpretation: see the case 8.
+ */
+ box_key_part_def_t a_13[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode_am_s3", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_13[] = {
+ {{1, 0, "string", "unicode_fi_s3", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_13[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode_am_s3", NULL}}, /* from <a> */
+ {{1, 0, "string", "unicode_fi_s3", NULL}}, /* from <b> */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_13, lengthof(a_13), b_13, lengthof(b_13),
+ exp_13, lengthof(exp_13));
+
+ /*
+ * Case 14: 'none' collation + ICU collation.
+ *
+ * Interpretation: see the case 8.
+ *
+ * Note: 'none' collation is the same as lack of a
+ * collation from key_def point of view. So after
+ * dump to key parts it becomes NULL.
+ */
+ box_key_part_def_t a_14[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "none", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_14[] = {
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_14[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", NULL, NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_14, lengthof(a_14), b_14, lengthof(b_14),
+ exp_14, lengthof(exp_14));
+
+ /*
+ * Case 15: ICU collation + 'none' collation.
+ *
+ * Interpretation: see the case 8.
+ *
+ * Note: 'none' collation is the same as lack of a
+ * collation from key_def point of view. So after
+ * dump to key parts it becomes NULL.
+ */
+ box_key_part_def_t a_15[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_15[] = {
+ {{1, 0, "string", "none", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_15[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* from <a> */
+ {{1, 0, "string", NULL, NULL}}, /* from <b> */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_15, lengthof(a_15), b_15, lengthof(b_15),
+ exp_15, lengthof(exp_15));
+
+ /*
+ * Case 16: 'binary' collation + ICU collation.
+ *
+ * Interpretation: see the case 8.
+ */
+ box_key_part_def_t a_16[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "binary", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_16[] = {
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_16[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "binary", NULL}}, /* coalesced */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_16, lengthof(a_16), b_16, lengthof(b_16),
+ exp_16, lengthof(exp_16));
+
+ /*
+ * Case 17: ICU collation + 'binary' collation.
+ *
+ * Interpretation: see the case 8.
+ */
+ box_key_part_def_t a_17[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* clash */
+ };
+ box_key_part_def_t b_17[] = {
+ {{1, 0, "string", "binary", NULL}}, /* clash */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ box_key_part_def_t exp_17[] = {
+ {{3, 0, "unsigned", NULL, NULL}},
+ {{1, 0, "string", "unicode", NULL}}, /* from <a> */
+ {{1, 0, "string", "binary", NULL}}, /* from <b> */
+ {{2, 0, "unsigned", NULL, NULL}},
+ };
+ key_def_check_merge(a_17, lengthof(a_17), b_17, lengthof(b_17),
+ exp_17, lengthof(exp_17));
+
+ /*
+ * Case 18: the same JSON paths.
+ *
+ * Interpretation: <fieldno> and <path> are considered as
+ * a 'pointer' to a field. JSON path are compared by its
+ * meaning, not just byte-to-byte. See also the case 3.
+ */
+ box_key_part_def_t a_18[] = {
+ {{0, 0, "unsigned", NULL, "moo"}},
+ };
+ box_key_part_def_t b_18[] = {
+ {{0, 0, "unsigned", NULL, "moo"}},
+ };
+ box_key_part_def_t exp_18[] = {
+ {{0, 0, "unsigned", NULL, "moo"}}, /* coalesced */
+ };
+ key_def_check_merge(a_18, lengthof(a_18), b_18, lengthof(b_18),
+ exp_18, lengthof(exp_18));
+
+ /*
+ * Case 19: the same JSON paths, but different <fieldno>.
+ *
+ * Interpretation: see the case 18.
+ */
+ box_key_part_def_t a_19[] = {
+ {{0, 0, "unsigned", NULL, "moo"}},
+ };
+ box_key_part_def_t b_19[] = {
+ {{1, 0, "unsigned", NULL, "moo"}},
+ };
+ box_key_part_def_t exp_19[] = {
+ {{0, 0, "unsigned", NULL, "moo"}},
+ {{1, 0, "unsigned", NULL, "moo"}},
+ };
+ key_def_check_merge(a_19, lengthof(a_19), b_19, lengthof(b_19),
+ exp_19, lengthof(exp_19));
+
+ /*
+ * Case 20: equivalent JSON paths.
+ *
+ * Interpretation: see the case 18. A key part from <b>
+ * is omitted in the case, so the JSON path from <a> is
+ * present in the result.
+ */
+ box_key_part_def_t a_20[] = {
+ {{0, 0, "unsigned", NULL, ".moo"}},
+ };
+ box_key_part_def_t b_20[] = {
+ {{0, 0, "unsigned", NULL, "moo" }},
+ };
+ box_key_part_def_t exp_20[] = {
+ {{0, 0, "unsigned", NULL, ".moo"}}, /* coalesced */
+ };
+ key_def_check_merge(a_20, lengthof(a_20), b_20, lengthof(b_20),
+ exp_20, lengthof(exp_20));
+
+ /*
+ * Case 21: no JSON path + JSON path.
+ *
+ * Interpretation: see the case 18.
+ */
+ box_key_part_def_t a_21[] = {
+ {{0, 0, "unsigned", NULL, NULL }},
+ };
+ box_key_part_def_t b_21[] = {
+ {{0, 0, "unsigned", NULL, "moo"}},
+ };
+ box_key_part_def_t exp_21[] = {
+ {{0, 0, "unsigned", NULL, NULL }},
+ {{0, 0, "unsigned", NULL, "moo"}},
+ };
+ key_def_check_merge(a_21, lengthof(a_21), b_21, lengthof(b_21),
+ exp_21, lengthof(exp_21));
+
+ /*
+ * Case 22: JSON path + no JSON path.
+ *
+ * Interpretation: see the case 18.
+ */
+ box_key_part_def_t a_22[] = {
+ {{0, 0, "unsigned", NULL, "moo"}},
+ };
+ box_key_part_def_t b_22[] = {
+ {{0, 0, "unsigned", NULL, NULL }},
+ };
+ box_key_part_def_t exp_22[] = {
+ {{0, 0, "unsigned", NULL, "moo"}},
+ {{0, 0, "unsigned", NULL, NULL }},
+ };
+ key_def_check_merge(a_22, lengthof(a_22), b_22, lengthof(b_22),
+ exp_22, lengthof(exp_22));
+
+ /*
+ * Case 23: different JSON paths.
+ *
+ * Interpretation: see the case 18.
+ */
+ box_key_part_def_t a_23[] = {
+ {{0, 0, "unsigned", NULL, "foo"}},
+ };
+ box_key_part_def_t b_23[] = {
+ {{0, 0, "unsigned", NULL, "bar"}},
+ };
+ box_key_part_def_t exp_23[] = {
+ {{0, 0, "unsigned", NULL, "foo"}},
+ {{0, 0, "unsigned", NULL, "bar"}},
+ };
+ key_def_check_merge(a_23, lengthof(a_23), b_23, lengthof(b_23),
+ exp_23, lengthof(exp_23));
+
+ /*
+ * Case 24: a shorter JSON path + a longer JSON path, but
+ * with the same prefix.
+ *
+ * Interpretation: see the case 18. Those JSON paths are
+ * not equivalent.
+ */
+ box_key_part_def_t a_24[] = {
+ {{0, 0, "unsigned", NULL, "foo" }},
+ };
+ box_key_part_def_t b_24[] = {
+ {{0, 0, "unsigned", NULL, "foo.bar"}},
+ };
+ box_key_part_def_t exp_24[] = {
+ {{0, 0, "unsigned", NULL, "foo" }},
+ {{0, 0, "unsigned", NULL, "foo.bar"}},
+ };
+ key_def_check_merge(a_24, lengthof(a_24), b_24, lengthof(b_24),
+ exp_24, lengthof(exp_24));
+
+ /*
+ * Case 25: a longer JSON path + a shorter JSON path, but
+ * with the same prefix.
+ *
+ * Interpretation: see the case 18. Those JSON paths are
+ * not equivalent.
+ */
+ box_key_part_def_t a_25[] = {
+ {{0, 0, "unsigned", NULL, "foo.bar"}},
+ };
+ box_key_part_def_t b_25[] = {
+ {{0, 0, "unsigned", NULL, "foo" }},
+ };
+ box_key_part_def_t exp_25[] = {
+ {{0, 0, "unsigned", NULL, "foo.bar"}},
+ {{0, 0, "unsigned", NULL, "foo" }},
+ };
+ key_def_check_merge(a_25, lengthof(a_25), b_25, lengthof(b_25),
+ exp_25, lengthof(exp_25));
+
+ /* Clean up. */
+ box_region_truncate(region_svp);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
/* }}} key_def api v2 */
static int
@@ -1209,6 +1960,7 @@ luaopen_module_api(lua_State *L)
{"test_key_def_new_v2", test_key_def_new_v2},
{"test_key_def_dump_parts", test_key_def_dump_parts},
{"test_key_def_validate_tuple", test_key_def_validate_tuple},
+ {"test_key_def_merge", test_key_def_merge},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index dbea9343b..6d045f8ce 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(31)
+ test:plan(32)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* [Tarantool-patches] [PATCH v3 14/16] module api: expose box_key_def_extract_key()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (12 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 13/16] module api: expose box_key_def_merge() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 15/16] module api: add box_key_def_validate_key() Alexander Turenko
` (4 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
Unlike box_tuple_extract_key() it accepts a key_def structure, not
space_id, index_id pair.
Another difference from box_tuple_extract_key() is that this function
allows to pass so called multikey index. See commit 2.2.0-259-gf1d9f2575
('box: introduce multikey indexes in memtx') for details.
Note: The <multikey_idx> parameter is ignored on the backported version
of the patch on 1.10.
Part of #5273
---
src/box/key_def.c | 7 ++
src/box/key_def.h | 20 +++++
src/exports.h | 1 +
test/app-tap/module_api.c | 126 +++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 155 insertions(+), 1 deletion(-)
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 7226f2482..da1c23135 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -620,6 +620,13 @@ box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
return key_def_merge(first, second);
}
+char *
+box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
+ int multikey_idx, uint32_t *key_size_ptr)
+{
+ return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
+}
+
/* }}} Module API functions */
int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index fdf65dec6..1b27836a8 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -525,6 +525,26 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
API_EXPORT box_key_def_t *
box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second);
+/**
+ * Extract key from tuple by given key definition and return
+ * buffer allocated on the box region with this key.
+ * @sa <box_region_truncate>().
+ *
+ * This function has O(n) complexity, where n is the number of key
+ * parts.
+ *
+ * @param key_def Definition of key that need to extract.
+ * @param tuple Tuple from which need to extract key.
+ * @param multikey_idx Multikey index hint or -1.
+ * @param key_size_ptr Here will be size of extracted key.
+ *
+ * @retval not NULL Success.
+ * @retval NULL Memory allocation error.
+ */
+API_EXPORT char *
+box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
+ int multikey_idx, uint32_t *key_size_ptr);
+
/** \endcond public */
/*
diff --git a/src/exports.h b/src/exports.h
index 223390d52..a0c7ac84d 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -31,6 +31,7 @@ EXPORT(box_iterator_free)
EXPORT(box_iterator_next)
EXPORT(box_key_def_delete)
EXPORT(box_key_def_dump_parts)
+EXPORT(box_key_def_extract_key)
EXPORT(box_key_def_merge)
EXPORT(box_key_def_new)
EXPORT(box_key_def_new_v2)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 175217ef9..25cd8a5e7 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1580,6 +1580,131 @@ test_key_def_merge(struct lua_State *L)
return 1;
}
+/**
+ * Basic <box_key_def_extract_key>() test.
+ */
+static int
+test_key_def_extract_key(struct lua_State *L)
+{
+ size_t region_svp = box_region_used();
+
+ /*
+ * Create a key_def.
+ *
+ * | tuple
+ * | [x, x, x]
+ * | key_def ^ ^
+ * | | | |
+ * | (0) <-----+---- string (optional)
+ * | | |
+ * | (1) <---- unsigned
+ */
+ box_key_part_def_t parts[2];
+ box_key_part_def_create(&parts[0]);
+ box_key_part_def_create(&parts[1]);
+ parts[0].fieldno = 2;
+ parts[0].field_type = "string";
+ parts[0].flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+ parts[1].fieldno = 0;
+ parts[1].field_type = "unsigned";
+ box_key_def_t *key_def = box_key_def_new_v2(parts, 2);
+ assert(key_def != NULL);
+
+ /*
+ * Create tuples to extract keys from them.
+ *
+ * | # | tuple | key |
+ * | - | ------------- | ---------- |
+ * | 0 | [1, 2, "moo"] | ["moo", 1] |
+ * | 1 | [1, 2, null] | [null, 1] |
+ * | 2 | [1, 2] | [null, 1] |
+ * | 3 | [1] | [null, 1] |
+ */
+ box_tuple_t *tuples[] = {
+ /* [0] = */ new_runtime_tuple("\x93\x01\x02\xa3moo", 7),
+ /* [1] = */ new_runtime_tuple("\x93\x01\x02\xc0", 4),
+ /* [2] = */ new_runtime_tuple("\x92\x01\x02", 3),
+ /* [3] = */ new_runtime_tuple("\x91\x01", 2),
+ };
+ struct {
+ const char *key;
+ uint32_t key_size;
+ } expected_keys_1[] = {
+ /* [0] = */ {"\x92\xa3moo\x01", 6},
+ /* [1] = */ {"\x92\xc0\x01", 3},
+ /* [2] = */ {"\x92\xc0\x01", 3},
+ /* [3] = */ {"\x92\xc0\x01", 3},
+ };
+
+ for (size_t i = 0; i < lengthof(tuples); ++i) {
+ uint32_t key_size = 0;
+ char *key = box_key_def_extract_key(key_def, tuples[i], -1,
+ &key_size);
+ assert(key != NULL);
+ uint32_t exp_key_size = expected_keys_1[i].key_size;
+ const char *exp_key = expected_keys_1[i].key;
+ assert(key_size == exp_key_size);
+ assert(memcmp(key, exp_key, exp_key_size) == 0);
+ }
+
+ /* Clean up. */
+ for (size_t i = 0; i < lengthof(tuples); ++i)
+ box_tuple_unref(tuples[i]);
+ box_key_def_delete(key_def);
+
+ /*
+ * Create a key_def with multikey JSON path.
+ *
+ * | tuple
+ * | [[x, x, x], x, x]
+ * | key_def ^ ^ ^
+ * | | 0 1 2
+ * | | | | |
+ * | | |--+--+
+ * | | |
+ * | (0) <---- unsigned
+ */
+ box_key_part_def_t part;
+ box_key_part_def_create(&part);
+ part.fieldno = 0;
+ part.field_type = "unsigned";
+ part.path = "[*]";
+ key_def = box_key_def_new_v2(&part, 1);
+ assert(key_def != NULL);
+
+ /* [[7, 2, 1], 5, 4] */
+ box_tuple_t *tuple =
+ new_runtime_tuple("\x93\x93\x07\x02\x01\x05\x04", 7);
+
+ struct {
+ const char *key;
+ uint32_t key_size;
+ } expected_keys_2[] = {
+ /* [0] = */ {"\x91\x07", 2},
+ /* [1] = */ {"\x91\x02", 2},
+ /* [2] = */ {"\x91\x01", 2},
+ };
+
+ for (int i = 0; i < (int)lengthof(expected_keys_2); ++i) {
+ uint32_t key_size = 0;
+ char *key = box_key_def_extract_key(key_def, tuple, i,
+ &key_size);
+ assert(key != NULL);
+ uint32_t exp_key_size = expected_keys_2[i].key_size;
+ const char *exp_key = expected_keys_2[i].key;
+ assert(key_size == exp_key_size);
+ assert(memcmp(key, exp_key, exp_key_size) == 0);
+ }
+
+ /* Clean up. */
+ box_tuple_unref(tuple);
+ box_key_def_delete(key_def);
+ box_region_truncate(region_svp);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
/* }}} key_def api v2 */
static int
@@ -1961,6 +2086,7 @@ luaopen_module_api(lua_State *L)
{"test_key_def_dump_parts", test_key_def_dump_parts},
{"test_key_def_validate_tuple", test_key_def_validate_tuple},
{"test_key_def_merge", test_key_def_merge},
+ {"test_key_def_extract_key", test_key_def_extract_key},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 6d045f8ce..4de450462 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(32)
+ test:plan(33)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 14/16] module api: expose box_key_def_extract_key()
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 14/16] module api: expose box_key_def_extract_key() Alexander Turenko
@ 2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-15 2:39 ` Alexander Turenko
0 siblings, 1 reply; 33+ messages in thread
From: Vladislav Shpilevoy @ 2020-10-14 23:41 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Thanks for the patch!
> diff --git a/src/box/key_def.c b/src/box/key_def.c
> index 7226f2482..da1c23135 100644
> --- a/src/box/key_def.c
> +++ b/src/box/key_def.c
> @@ -620,6 +620,13 @@ box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
> return key_def_merge(first, second);
> }
>
> +char *
> +box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
> + int multikey_idx, uint32_t *key_size_ptr)
I would try to make the tuple const here. Key_def can't be const (because
multikey updates some values in it even in simple getters), but the tuple
can. Although it requires const addition to several internal functions.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 14/16] module api: expose box_key_def_extract_key()
2020-10-14 23:41 ` Vladislav Shpilevoy
@ 2020-10-15 2:39 ` Alexander Turenko
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-15 2:39 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Thu, Oct 15, 2020 at 01:41:51AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> > diff --git a/src/box/key_def.c b/src/box/key_def.c
> > index 7226f2482..da1c23135 100644
> > --- a/src/box/key_def.c
> > +++ b/src/box/key_def.c
> > @@ -620,6 +620,13 @@ box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second)
> > return key_def_merge(first, second);
> > }
> >
> > +char *
> > +box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
> > + int multikey_idx, uint32_t *key_size_ptr)
>
> I would try to make the tuple const here. Key_def can't be const (because
> multikey updates some values in it even in simple getters), but the tuple
> can. Although it requires const addition to several internal functions.
A fear to do so after Vladimir's commit
077671fe04b5fec898cccecd4aac12ee4281ce0b ('Drop const qualifier of
struct tuple').
In theory: what if we'll have some offset map that will be calculated
lazily for some kind of tuples?
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Tarantool-patches] [PATCH v3 15/16] module api: add box_key_def_validate_key()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (13 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 14/16] module api: expose box_key_def_extract_key() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 16/16] module api: add box_key_def_validate_full_key() Alexander Turenko
` (3 subsequent siblings)
18 siblings, 1 reply; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
The function allows to verify a key against a key definition. It accepts
a partial key.
Part of #5273
---
src/box/key_def.c | 13 +++++
src/box/key_def.h | 21 ++++++++
src/exports.h | 1 +
test/app-tap/module_api.c | 88 ++++++++++++++++++++++++++++++++
test/app-tap/module_api.test.lua | 2 +-
5 files changed, 124 insertions(+), 1 deletion(-)
diff --git a/src/box/key_def.c b/src/box/key_def.c
index da1c23135..55ac79362 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -627,6 +627,19 @@ box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
}
+int
+box_key_def_validate_key(const box_key_def_t *key_def, const char *key)
+{
+ uint32_t part_count = mp_decode_array(&key);
+ if (part_count > key_def->part_count) {
+ diag_set(ClientError, ER_KEY_PART_COUNT, key_def->part_count,
+ part_count);
+ return -1;
+ }
+ const char *key_end;
+ return key_validate_parts(key_def, key, part_count, true, &key_end);
+}
+
/* }}} Module API functions */
int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 1b27836a8..440f2fb13 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -545,6 +545,27 @@ API_EXPORT char *
box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
int multikey_idx, uint32_t *key_size_ptr);
+/**
+ * Check a key against given key definition.
+ *
+ * Verifies key parts against given key_def's field types with
+ * respect to nullability.
+ *
+ * A partial key (with less part than defined in @a key_def) is
+ * verified by given key parts, the omitted tail is not verified
+ * anyhow.
+ *
+ * Note: nil is accepted for nullable fields, but only for them.
+ *
+ * @param key_def Key definition.
+ * @param key MessagePack'ed data for matching.
+ *
+ * @retval 0 The key is valid.
+ * @retval -1 The key is invalid.
+ */
+API_EXPORT int
+box_key_def_validate_key(const box_key_def_t *key_def, const char *key);
+
/** \endcond public */
/*
diff --git a/src/exports.h b/src/exports.h
index a0c7ac84d..1d7deb518 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -35,6 +35,7 @@ EXPORT(box_key_def_extract_key)
EXPORT(box_key_def_merge)
EXPORT(box_key_def_new)
EXPORT(box_key_def_new_v2)
+EXPORT(box_key_def_validate_key)
EXPORT(box_key_def_validate_tuple)
EXPORT(box_key_part_def_create)
EXPORT(box_latch_delete)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 25cd8a5e7..778e7c3c0 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1705,6 +1705,93 @@ test_key_def_extract_key(struct lua_State *L)
return 1;
}
+/**
+ * Basic <box_key_def_validate_key>() test.
+ */
+static int
+test_key_def_validate_key(struct lua_State *L)
+{
+ /*
+ * Create a key_def.
+ *
+ * | tuple
+ * | [x, x, x]
+ * | key_def ^ ^
+ * | | | |
+ * | (0) <-----+---- unsigned
+ * | | |
+ * | (1) <---- unsigned (optional)
+ */
+ box_key_part_def_t parts[2];
+ box_key_part_def_create(&parts[0]);
+ box_key_part_def_create(&parts[1]);
+ parts[0].fieldno = 2;
+ parts[0].field_type = "unsigned";
+ parts[1].fieldno = 0;
+ parts[1].field_type = "unsigned";
+ parts[1].flags |= BOX_KEY_PART_DEF_IS_NULLABLE;
+ box_key_def_t *key_def = box_key_def_new_v2(parts, 2);
+ assert(key_def != NULL);
+
+ /*
+ * Create keys to validate them against given key_def.
+ *
+ * | # | key | Is valid? |
+ * | - | -------------- | --------- |
+ * | 0 | [1, 1] | valid |
+ * | 1 | [1, null] | valid |
+ * | 2 | [1] | valid |
+ * | 3 | [] | valid |
+ * | 4 | [null] | invalid |
+ * | 5 | [1, 2, 3] | invalid |
+ * | 6 | [1, -1] | invalid |
+ */
+ const char *keys[] = {
+ /* [0] = */ "\x92\x01\x01",
+ /* [1] = */ "\x92\x01\xc0",
+ /* [2] = */ "\x91\x01",
+ /* [3] = */ "\x90",
+ /* [4] = */ "\x91\xc0",
+ /* [5] = */ "\x93\x01\x02\x03",
+ /* [6] = */ "\x92\x01\xff",
+ };
+ int expected_results[] = {
+ /* [0] = */ 0,
+ /* [1] = */ 0,
+ /* [2] = */ 0,
+ /* [3] = */ 0,
+ /* [4] = */ -1,
+ /* [5] = */ -1,
+ /* [6] = */ -1,
+ };
+ uint32_t expected_error_codes[] = {
+ /* [0] = */ box_error_code_MAX,
+ /* [1] = */ box_error_code_MAX,
+ /* [2] = */ box_error_code_MAX,
+ /* [3] = */ box_error_code_MAX,
+ /* [4] = */ ER_KEY_PART_TYPE,
+ /* [5] = */ ER_KEY_PART_COUNT,
+ /* [6] = */ ER_KEY_PART_TYPE,
+ };
+
+ for (size_t i = 0; i < lengthof(keys); ++i) {
+ int rc = box_key_def_validate_key(key_def, keys[i]);
+ assert(rc == expected_results[i]);
+
+ if (expected_error_codes[i] != box_error_code_MAX) {
+ assert(rc != 0);
+ box_error_t *e = box_error_last();
+ assert(box_error_code(e) == expected_error_codes[i]);
+ }
+ }
+
+ /* Clean up. */
+ box_key_def_delete(key_def);
+
+ lua_pushboolean(L, 1);
+ return 1;
+}
+
/* }}} key_def api v2 */
static int
@@ -2087,6 +2174,7 @@ luaopen_module_api(lua_State *L)
{"test_key_def_validate_tuple", test_key_def_validate_tuple},
{"test_key_def_merge", test_key_def_merge},
{"test_key_def_extract_key", test_key_def_extract_key},
+ {"test_key_def_validate_key", test_key_def_validate_key},
{NULL, NULL}
};
luaL_register(L, "module_api", lib);
diff --git a/test/app-tap/module_api.test.lua b/test/app-tap/module_api.test.lua
index 4de450462..4e06cf431 100755
--- a/test/app-tap/module_api.test.lua
+++ b/test/app-tap/module_api.test.lua
@@ -177,7 +177,7 @@ local function test_iscdata(test, module)
end
local test = require('tap').test("module_api", function(test)
- test:plan(33)
+ test:plan(34)
local status, module = pcall(require, 'module_api')
test:is(status, true, "module")
test:ok(status, "module is loaded")
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 15/16] module api: add box_key_def_validate_key()
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 15/16] module api: add box_key_def_validate_key() Alexander Turenko
@ 2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-15 13:18 ` Alexander Turenko
0 siblings, 1 reply; 33+ messages in thread
From: Vladislav Shpilevoy @ 2020-10-14 23:41 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Thanks for the patch!
See 2 comments below.
On 13.10.2020 01:23, Alexander Turenko wrote:
> The function allows to verify a key against a key definition. It accepts
> a partial key.
>
> Part of #5273
> ---
> src/box/key_def.c | 13 +++++
> src/box/key_def.h | 21 ++++++++
> src/exports.h | 1 +
> test/app-tap/module_api.c | 88 ++++++++++++++++++++++++++++++++
> test/app-tap/module_api.test.lua | 2 +-
> 5 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/key_def.c b/src/box/key_def.c
> index da1c23135..55ac79362 100644
> --- a/src/box/key_def.c
> +++ b/src/box/key_def.c
> @@ -627,6 +627,19 @@ box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
> return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
> }
>
> +int
> +box_key_def_validate_key(const box_key_def_t *key_def, const char *key)
> +{
> + uint32_t part_count = mp_decode_array(&key);
> + if (part_count > key_def->part_count) {
> + diag_set(ClientError, ER_KEY_PART_COUNT, key_def->part_count,
> + part_count);
> + return -1;
> + }
> + const char *key_end;
1. Don't you want to make it an out parameter? We won't be able to add it
later, in case it will be ever needed.
> + return key_validate_parts(key_def, key, part_count, true, &key_end);
> +}
> +
> /* }}} Module API functions */
>
> diff --git a/src/box/key_def.h b/src/box/key_def.h
> index 1b27836a8..440f2fb13 100644
> --- a/src/box/key_def.h
> +++ b/src/box/key_def.h
> @@ -545,6 +545,27 @@ API_EXPORT char *
> box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
> int multikey_idx, uint32_t *key_size_ptr);
>
> +/**
> + * Check a key against given key definition.
> + *
> + * Verifies key parts against given key_def's field types with
> + * respect to nullability.
> + *
> + * A partial key (with less part than defined in @a key_def) is
> + * verified by given key parts, the omitted tail is not verified
> + * anyhow.
> + *
> + * Note: nil is accepted for nullable fields, but only for them.
> + *
> + * @param key_def Key definition.
> + * @param key MessagePack'ed data for matching.
> + *
> + * @retval 0 The key is valid.
> + * @retval -1 The key is invalid.
2. Maybe it is worth saying in all the new public functions, that
if they return -1, it means diag is set, and is available via
box_error_message() and shit.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 15/16] module api: add box_key_def_validate_key()
2020-10-14 23:41 ` Vladislav Shpilevoy
@ 2020-10-15 13:18 ` Alexander Turenko
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-15 13:18 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Thu, Oct 15, 2020 at 01:41:50AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
> On 13.10.2020 01:23, Alexander Turenko wrote:
> > The function allows to verify a key against a key definition. It accepts
> > a partial key.
> >
> > Part of #5273
> > ---
> > src/box/key_def.c | 13 +++++
> > src/box/key_def.h | 21 ++++++++
> > src/exports.h | 1 +
> > test/app-tap/module_api.c | 88 ++++++++++++++++++++++++++++++++
> > test/app-tap/module_api.test.lua | 2 +-
> > 5 files changed, 124 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/box/key_def.c b/src/box/key_def.c
> > index da1c23135..55ac79362 100644
> > --- a/src/box/key_def.c
> > +++ b/src/box/key_def.c
> > @@ -627,6 +627,19 @@ box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
> > return tuple_extract_key(tuple, key_def, multikey_idx, key_size_ptr);
> > }
> >
> > +int
> > +box_key_def_validate_key(const box_key_def_t *key_def, const char *key)
> > +{
> > + uint32_t part_count = mp_decode_array(&key);
> > + if (part_count > key_def->part_count) {
> > + diag_set(ClientError, ER_KEY_PART_COUNT, key_def->part_count,
> > + part_count);
> > + return -1;
> > + }
> > + const char *key_end;
>
> 1. Don't you want to make it an out parameter? We won't be able to add it
> later, in case it will be ever needed.
It seems to be quite specific case: when we validate an array of keys.
However, okay, why not?
I'll do it consistent with _extract_key(): <uint32_t *key_size_ptr>.
I'll also eliminate all those problems with const, when a caller-side
<char *key_end> is infected by const and must be <const char *key_end>.
It'll be a bit intruisive on 1.10: several usages of
key_validate_parts() must be updated like it was done on 2.* somewhere
around key_list introduction.
Added on the branches (for box_key_def_validate_full_key() as well).
The change looks so on master:
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 55ac79362..ca8a00dc5 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -628,16 +628,20 @@ box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
}
int
-box_key_def_validate_key(const box_key_def_t *key_def, const char *key)
+box_key_def_validate_key(const box_key_def_t *key_def, const char *key,
+ uint32_t *key_size_ptr)
{
- uint32_t part_count = mp_decode_array(&key);
+ const char *pos = key;
+ uint32_t part_count = mp_decode_array(&pos);
if (part_count > key_def->part_count) {
diag_set(ClientError, ER_KEY_PART_COUNT, key_def->part_count,
part_count);
return -1;
}
- const char *key_end;
- return key_validate_parts(key_def, key, part_count, true, &key_end);
+ int rc = key_validate_parts(key_def, pos, part_count, true, &pos);
+ if (rc == 0 && key_size_ptr != NULL)
+ *key_size_ptr = pos - key;
+ return rc;
}
/* }}} Module API functions */
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 8d942ee8f..9a0339757 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -557,14 +557,19 @@ box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
*
* Note: nil is accepted for nullable fields, but only for them.
*
- * @param key_def Key definition.
- * @param key MessagePack'ed data for matching.
+ * @param key_def Key definition.
+ * @param key MessagePack'ed data for matching.
+ * @param key_size_ptr Here will be size of the validated key.
*
* @retval 0 The key is valid.
* @retval -1 The key is invalid.
+ *
+ * In case of an error set a diag and return -1.
+ * @sa <box_error_last>().
*/
API_EXPORT int
-box_key_def_validate_key(const box_key_def_t *key_def, const char *key);
+box_key_def_validate_key(const box_key_def_t *key_def, const char *key,
+ uint32_t *key_size_ptr);
/** \endcond public */
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 778e7c3c0..f13b4be38 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1746,14 +1746,17 @@ test_key_def_validate_key(struct lua_State *L)
* | 5 | [1, 2, 3] | invalid |
* | 6 | [1, -1] | invalid |
*/
- const char *keys[] = {
- /* [0] = */ "\x92\x01\x01",
- /* [1] = */ "\x92\x01\xc0",
- /* [2] = */ "\x91\x01",
- /* [3] = */ "\x90",
- /* [4] = */ "\x91\xc0",
- /* [5] = */ "\x93\x01\x02\x03",
- /* [6] = */ "\x92\x01\xff",
+ struct {
+ const char *data;
+ uint32_t size;
+ } keys[] = {
+ /* [0] = */ {"\x92\x01\x01", 3},
+ /* [1] = */ {"\x92\x01\xc0", 3},
+ /* [2] = */ {"\x91\x01", 2},
+ /* [3] = */ {"\x90", 1},
+ /* [4] = */ {"\x91\xc0", 2},
+ /* [5] = */ {"\x93\x01\x02\x03", 4},
+ /* [6] = */ {"\x92\x01\xff", 3},
};
int expected_results[] = {
/* [0] = */ 0,
@@ -1775,10 +1778,23 @@ test_key_def_validate_key(struct lua_State *L)
};
for (size_t i = 0; i < lengthof(keys); ++i) {
- int rc = box_key_def_validate_key(key_def, keys[i]);
+ uint32_t key_size = 0;
+ const char *key = keys[i].data;
+ int rc = box_key_def_validate_key(key_def, key, &key_size);
assert(rc == expected_results[i]);
- if (expected_error_codes[i] != box_error_code_MAX) {
+ if (expected_error_codes[i] == box_error_code_MAX) {
+ /* Verify key_size. */
+ assert(key_size != 0);
+ assert(key_size == keys[i].size);
+
+ /*
+ * Verify that no NULL pointer dereference
+ * occurs when NULL is passed as
+ * key_size_ptr.
+ */
+ box_key_def_validate_key(key_def, key, NULL);
+ } else {
assert(rc != 0);
box_error_t *e = box_error_last();
assert(box_error_code(e) == expected_error_codes[i]);
>
> > + return key_validate_parts(key_def, key, part_count, true, &key_end);
> > +}
> > +
> > /* }}} Module API functions */
> >
> > diff --git a/src/box/key_def.h b/src/box/key_def.h
> > index 1b27836a8..440f2fb13 100644
> > --- a/src/box/key_def.h
> > +++ b/src/box/key_def.h
> > @@ -545,6 +545,27 @@ API_EXPORT char *
> > box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
> > int multikey_idx, uint32_t *key_size_ptr);
> >
> > +/**
> > + * Check a key against given key definition.
> > + *
> > + * Verifies key parts against given key_def's field types with
> > + * respect to nullability.
> > + *
> > + * A partial key (with less part than defined in @a key_def) is
> > + * verified by given key parts, the omitted tail is not verified
> > + * anyhow.
> > + *
> > + * Note: nil is accepted for nullable fields, but only for them.
> > + *
> > + * @param key_def Key definition.
> > + * @param key MessagePack'ed data for matching.
> > + *
> > + * @retval 0 The key is valid.
> > + * @retval -1 The key is invalid.
>
> 2. Maybe it is worth saying in all the new public functions, that
> if they return -1, it means diag is set, and is available via
> box_error_message() and shit.
Sure.
Added phrases as follows on the branches:
| In case of an error set a diag and return -1.
| @sa <box_error_last>().
| In case of an error set a diag and return NULL.
| @sa <box_error_last>().
| In case of an invalid tuple set a diag and return -1.
| @sa <box_error_last>().
| In case of an invalid key set a diag and return -1.
| @sa <box_error_last>().
The list of functions, where I did it:
- box_key_def_new_v2
- box_key_def_dump_parts
- box_key_def_validate_tuple
- box_key_def_merge
- box_key_def_extract_key
- box_key_def_validate_key
- box_key_def_validate_full_key
The rest of added functions already mention the diagnostics area
somehow.
Aside of this I observed that box_region_alloc() and
box_region_aligned_alloc() must also set a diag, because they're box api
functions. Fixed this on the branches.
diff --git a/src/lib/core/fiber.c b/src/lib/core/fiber.c
index 33ec47c66..fab7740b2 100644
--- a/src/lib/core/fiber.c
+++ b/src/lib/core/fiber.c
@@ -1377,13 +1377,19 @@ box_region_used(void)
void *
box_region_alloc(size_t size)
{
- return region_alloc(&fiber()->gc, size);
+ void *res = region_alloc(&fiber()->gc, size);
+ if (res == NULL)
+ diag_set(OutOfMemory, size, "region_alloc", "data");
+ return res;
}
void *
box_region_aligned_alloc(size_t size, size_t alignment)
{
- return region_aligned_alloc(&fiber()->gc, size, alignment);
+ void *res = region_aligned_alloc(&fiber()->gc, size, alignment);
+ if (res == NULL)
+ diag_set(OutOfMemory, size, "region_alloc", "aligned data");
+ return res;
}
void
diff --git a/src/lib/core/fiber.h b/src/lib/core/fiber.h
index ca3028b20..539e5c8e7 100644
--- a/src/lib/core/fiber.h
+++ b/src/lib/core/fiber.h
@@ -444,6 +444,9 @@ box_region_used(void);
* or array of values of a type with alignment requirements. A
* violation of alignment requirements leads to undefined
* behaviour.
+ *
+ * In case of a memory error set a diag and return NULL.
+ * @sa <box_error_last>().
*/
API_EXPORT void *
box_region_alloc(size_t size);
@@ -452,6 +455,9 @@ box_region_alloc(size_t size);
* Allocate size bytes from the box region with given alignment.
*
* Alignment must be a power of 2.
+ *
+ * In case of a memory error set a diag and return NULL.
+ * @sa <box_error_last>().
*/
API_EXPORT void *
box_region_aligned_alloc(size_t size, size_t alignment);
^ permalink raw reply [flat|nested] 33+ messages in thread
* [Tarantool-patches] [PATCH v3 16/16] module api: add box_key_def_validate_full_key()
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (14 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 15/16] module api: add box_key_def_validate_key() Alexander Turenko
@ 2020-10-12 23:23 ` Alexander Turenko
2020-10-14 23:41 ` [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Vladislav Shpilevoy
` (2 subsequent siblings)
18 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-12 23:23 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches, Alexander Turenko
box_key_def_validate_key() verifies a probably partial key, while the
new function is to apply the extra restriction on the key part count: it
should be the same as in a given key definition.
Fixes #5273
---
src/box/key_def.c | 13 +++++++
src/box/key_def.h | 20 ++++++++++
src/exports.h | 1 +
test/app-tap/module_api.c | 79 +++++++++++++++++++++++----------------
4 files changed, 80 insertions(+), 33 deletions(-)
diff --git a/src/box/key_def.c b/src/box/key_def.c
index 55ac79362..3df67fa3b 100644
--- a/src/box/key_def.c
+++ b/src/box/key_def.c
@@ -640,6 +640,19 @@ box_key_def_validate_key(const box_key_def_t *key_def, const char *key)
return key_validate_parts(key_def, key, part_count, true, &key_end);
}
+int
+box_key_def_validate_full_key(const box_key_def_t *key_def, const char *key)
+{
+ uint32_t part_count = mp_decode_array(&key);
+ if (part_count != key_def->part_count) {
+ diag_set(ClientError, ER_EXACT_MATCH, key_def->part_count,
+ part_count);
+ return -1;
+ }
+ const char *key_end;
+ return key_validate_parts(key_def, key, part_count, true, &key_end);
+}
+
/* }}} Module API functions */
int
diff --git a/src/box/key_def.h b/src/box/key_def.h
index 440f2fb13..ec60152d1 100644
--- a/src/box/key_def.h
+++ b/src/box/key_def.h
@@ -566,6 +566,26 @@ box_key_def_extract_key(box_key_def_t *key_def, box_tuple_t *tuple,
API_EXPORT int
box_key_def_validate_key(const box_key_def_t *key_def, const char *key);
+/**
+ * Check a full key against given key definition.
+ *
+ * Verifies key parts against given key_def's field types with
+ * respect to nullability.
+ *
+ * Imposes the same parts count in @a key as in @a key_def.
+ * Absense of trailing key parts fails the check.
+ *
+ * Note: nil is accepted for nullable fields, but only for them.
+ *
+ * @param key_def Key definition.
+ * @param key MessagePack'ed data for matching.
+ *
+ * @retval 0 The key is valid.
+ * @retval -1 The key is invalid.
+ */
+API_EXPORT int
+box_key_def_validate_full_key(const box_key_def_t *key_def, const char *key);
+
/** \endcond public */
/*
diff --git a/src/exports.h b/src/exports.h
index 1d7deb518..ad5f3f08e 100644
--- a/src/exports.h
+++ b/src/exports.h
@@ -35,6 +35,7 @@ EXPORT(box_key_def_extract_key)
EXPORT(box_key_def_merge)
EXPORT(box_key_def_new)
EXPORT(box_key_def_new_v2)
+EXPORT(box_key_def_validate_full_key)
EXPORT(box_key_def_validate_key)
EXPORT(box_key_def_validate_tuple)
EXPORT(box_key_part_def_create)
diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c
index 778e7c3c0..a3c4850cd 100644
--- a/test/app-tap/module_api.c
+++ b/test/app-tap/module_api.c
@@ -1706,7 +1706,8 @@ test_key_def_extract_key(struct lua_State *L)
}
/**
- * Basic <box_key_def_validate_key>() test.
+ * Basic <box_key_def_validate_key>() and
+ * <box_key_def_validate_full_key>() test.
*/
static int
test_key_def_validate_key(struct lua_State *L)
@@ -1736,15 +1737,16 @@ test_key_def_validate_key(struct lua_State *L)
/*
* Create keys to validate them against given key_def.
*
- * | # | key | Is valid? |
- * | - | -------------- | --------- |
- * | 0 | [1, 1] | valid |
- * | 1 | [1, null] | valid |
- * | 2 | [1] | valid |
- * | 3 | [] | valid |
- * | 4 | [null] | invalid |
- * | 5 | [1, 2, 3] | invalid |
- * | 6 | [1, -1] | invalid |
+ * | # | key | Is valid? | Is valid? |
+ * | | | (partial) | (full) |
+ * | - | -------------- | --------- | --------- |
+ * | 0 | [1, 1] | valid | valid |
+ * | 1 | [1, null] | valid | valid |
+ * | 2 | [1] | valid | invalid |
+ * | 3 | [] | valid | invalid |
+ * | 4 | [null] | invalid | invalid |
+ * | 5 | [1, 2, 3] | invalid | invalid |
+ * | 6 | [1, -1] | invalid | invalid |
*/
const char *keys[] = {
/* [0] = */ "\x92\x01\x01",
@@ -1755,33 +1757,44 @@ test_key_def_validate_key(struct lua_State *L)
/* [5] = */ "\x93\x01\x02\x03",
/* [6] = */ "\x92\x01\xff",
};
- int expected_results[] = {
- /* [0] = */ 0,
- /* [1] = */ 0,
- /* [2] = */ 0,
- /* [3] = */ 0,
- /* [4] = */ -1,
- /* [5] = */ -1,
- /* [6] = */ -1,
+ int expected_results[][2] = {
+ /* [0] = */ {0, 0 },
+ /* [1] = */ {0, 0 },
+ /* [2] = */ {0, -1},
+ /* [3] = */ {0, -1},
+ /* [4] = */ {-1, -1},
+ /* [5] = */ {-1, -1},
+ /* [6] = */ {-1, -1},
};
- uint32_t expected_error_codes[] = {
- /* [0] = */ box_error_code_MAX,
- /* [1] = */ box_error_code_MAX,
- /* [2] = */ box_error_code_MAX,
- /* [3] = */ box_error_code_MAX,
- /* [4] = */ ER_KEY_PART_TYPE,
- /* [5] = */ ER_KEY_PART_COUNT,
- /* [6] = */ ER_KEY_PART_TYPE,
+ uint32_t expected_error_codes[][2] = {
+ /* [0] = */ {box_error_code_MAX, box_error_code_MAX},
+ /* [1] = */ {box_error_code_MAX, box_error_code_MAX},
+ /* [2] = */ {box_error_code_MAX, ER_EXACT_MATCH },
+ /* [3] = */ {box_error_code_MAX, ER_EXACT_MATCH },
+ /* [4] = */ {ER_KEY_PART_TYPE, ER_EXACT_MATCH },
+ /* [5] = */ {ER_KEY_PART_COUNT, ER_EXACT_MATCH },
+ /* [6] = */ {ER_KEY_PART_TYPE, ER_KEY_PART_TYPE },
};
- for (size_t i = 0; i < lengthof(keys); ++i) {
- int rc = box_key_def_validate_key(key_def, keys[i]);
- assert(rc == expected_results[i]);
+ typedef int (*key_def_validate_key_f)(const box_key_def_t *key_def,
+ const char *key);
+ key_def_validate_key_f funcs[] = {
+ box_key_def_validate_key,
+ box_key_def_validate_full_key,
+ };
- if (expected_error_codes[i] != box_error_code_MAX) {
- assert(rc != 0);
- box_error_t *e = box_error_last();
- assert(box_error_code(e) == expected_error_codes[i]);
+ for (size_t i = 0; i < lengthof(keys); ++i) {
+ for (size_t f = 0; f < lengthof(funcs); ++f) {
+ int exp_res = expected_results[i][f];
+ uint32_t exp_err_code = expected_error_codes[i][f];
+ int rc = funcs[f](key_def, keys[i]);
+ assert(rc == exp_res);
+
+ if (exp_err_code != box_error_code_MAX) {
+ assert(rc != 0);
+ box_error_t *e = box_error_last();
+ assert(box_error_code(e) == exp_err_code);
+ }
}
}
--
2.25.0
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (15 preceding siblings ...)
2020-10-12 23:23 ` [Tarantool-patches] [PATCH v3 16/16] module api: add box_key_def_validate_full_key() Alexander Turenko
@ 2020-10-14 23:41 ` Vladislav Shpilevoy
2020-10-15 3:09 ` Alexander Turenko
2020-10-15 13:19 ` Alexander Turenko
2020-10-15 20:12 ` Vladislav Shpilevoy
18 siblings, 1 reply; 33+ messages in thread
From: Vladislav Shpilevoy @ 2020-10-14 23:41 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Hi! Thanks for the patchset!
Are we going to create a docbot request or a documentation ticket
for all these newly exported functions?
> Alexander Turenko (16):
> module api: get rid of typedef redefinitions
> module api: expose box region
> module api/lua: add luaL_iscdata() function
> lua: factor out tuple encoding from luaT_tuple_new
> lua: don't raise a Lua error from luaT_tuple_new()
> module api/lua: add luaT_tuple_encode()
> module api/lua: expose luaT_tuple_new()
> module api/lua: add API_EXPORT to tuple functions
> module api: add API_EXPORT to key_def functions
> module api: add box_key_def_new_v2()
> module api: add box_key_def_dump_parts()
> module api: expose box_key_def_validate_tuple()
> module api: expose box_key_def_merge()
> module api: expose box_key_def_extract_key()
> module api: add box_key_def_validate_key()
> module api: add box_key_def_validate_full_key()
Still don't understand why do we need full key validation.
Anyway it won't help in anything. The only purpose I can
think of is kind of a guarantee, that if a key is full and
valid, it will return at most 1 tuple, but it is not so for
non-unique indexes - for them a full and not full keys are
the same. But whatever.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module
2020-10-14 23:41 ` [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Vladislav Shpilevoy
@ 2020-10-15 3:09 ` Alexander Turenko
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-15 3:09 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Thu, Oct 15, 2020 at 01:41:57AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
>
> Are we going to create a docbot request or a documentation ticket
> for all these newly exported functions?
I will create the issue manually.
>
> > Alexander Turenko (16):
> > module api: get rid of typedef redefinitions
> > module api: expose box region
> > module api/lua: add luaL_iscdata() function
> > lua: factor out tuple encoding from luaT_tuple_new
> > lua: don't raise a Lua error from luaT_tuple_new()
> > module api/lua: add luaT_tuple_encode()
> > module api/lua: expose luaT_tuple_new()
> > module api/lua: add API_EXPORT to tuple functions
> > module api: add API_EXPORT to key_def functions
> > module api: add box_key_def_new_v2()
> > module api: add box_key_def_dump_parts()
> > module api: expose box_key_def_validate_tuple()
> > module api: expose box_key_def_merge()
> > module api: expose box_key_def_extract_key()
> > module api: add box_key_def_validate_key()
> > module api: add box_key_def_validate_full_key()
>
> Still don't understand why do we need full key validation.
> Anyway it won't help in anything. The only purpose I can
> think of is kind of a guarantee, that if a key is full and
> valid, it will return at most 1 tuple, but it is not so for
> non-unique indexes - for them a full and not full keys are
> the same. But whatever.
But what if a caller knows that a particular key_def corresponds to some
unique index and want to ensure that a key is not only valid, but also
full? Or it knows that a particular key_def imposes a strict order on a
particular tuple stream and want to ensure that compare against given
key will impose the same strict ordering? It may be important for
pagination.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (16 preceding siblings ...)
2020-10-14 23:41 ` [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Vladislav Shpilevoy
@ 2020-10-15 13:19 ` Alexander Turenko
2020-10-16 6:05 ` Alexander Turenko
2020-10-15 20:12 ` Vladislav Shpilevoy
18 siblings, 1 reply; 33+ messages in thread
From: Alexander Turenko @ 2020-10-15 13:19 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Updated.
Changes from v3 to the final version (I hope it is final):
- Fixed sorting of the box region functions in src/exports.h.
- Added forgotten API_EXPORT for box_region_truncate().
- Added a comment for luaT_tuple_encode_table() reference in the Lua registry.
- Added the <size_t *key_size_ptr> parameter to box_key_def_validate_key() and
box_key_def_validate_full_key().
- Fixed API comments for key_def functions with mention of the diagnostics
area.
- Set a memory error to the diagnostics area for box_region_*alloc() functions.
On Tue, Oct 13, 2020 at 02:23:07AM +0300, Alexander Turenko wrote:
> Vlad,
>
> Thank you for the past thorough reviews and answering to a lot of my
> questions!
>
> Can you look over the updated patchset once again?
>
> Overview
> --------
>
> The patchset exposes and adds new functions into the module API, which
> are necessary to implement the external tuple.keydef module. Aside of
> this, a couple of relevant bugs were fixed.
>
> The extra commits for tarantool-1.10 are the following:
>
> - lua: adjust contract of luaT_tuple_new()
> - collation: allow to find a collation by a name
>
> I'll not flood the mailing list with the 1.10 version of the patchset
> (if someone will not ask me do to that). It mostly the same or similar
> as this patchset (however there are some code differences).
>
> Previous versions:
>
> [v1]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-September/019583.html
> [v2]: https://lists.tarantool.org/pipermail/tarantool-patches/2020-October/020019.html
>
> Changelog from v2 to v3
> -----------------------
>
> In brief: the last three commits from v2 gains test cases, the new
> function box_key_def_validate_full_key() is added.
>
> - Clarified how default values of <box_key_part_def_t> may be changed in a
> future tarantool versions.
> - Added a test case for 'none' collation for box_key_def_dump_parts().
> - Added -g to build module API test to obtain backtraces from GDB.
> - Added box_key_def_merge() test.
> - Added basic box_key_def_extract_key() test.
> - Added basic box_key_def_validate_key() test.
> - Clarified box_key_def_validate_key() API comment.
> - Fixed some typos, made suggested style changes.
> - Added region_truncate() in case of encoding or OOM error inside
> luaT_tuple_encode().
> - Added box_key_def_validate_full_key().
> - Clafiried the Lua stack manipulations in luaT_tuple_encode_on_lua_ibuf().
>
> @ChangeLog
>
> - Module API: Get rid of typedef redefinitions for compatibility with
> C99 (gh-5313).
> - Lua: Fixed unhandled Lua error that may lead to memory leaks and
> inconsistencies in `<space_object>:frommap()`,
> `<key_def_object>:compare()`, `<merge_source>:select()` (gh-5382).
> - Module API: Exposed the box region, key_def and several other
> functions in order to implement an external tuple.keydef module on top
> of them (gh-5273).
>
> Issues
> ------
>
> https://github.com/tarantool/tarantool/issues/5313 ('module api: module
> API requires C11')
> https://github.com/tarantool/tarantool/issues/5382 ('luaT_tuple_new()
> raises a Lua error that leads to various problems')
> https://github.com/tarantool/tarantool/issues/5273 ('module api: expose
> everything that is needed for external key_def module')
>
> Branches
> --------
>
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-5273-expand-module-api
> (top 16 commits)
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-5273-expand-module-api-1.10
> (top 18 commits)
>
> The module based on this API
> ----------------------------
>
> https://github.com/Totktonada/key_def
>
> The module repository will be moved to the tarantool organization soon,
> when the module API will come into the server. It'll be in
> https://github.com/tarantool/tuple-keydef
>
> Alexander Turenko (16):
> module api: get rid of typedef redefinitions
> module api: expose box region
> module api/lua: add luaL_iscdata() function
> lua: factor out tuple encoding from luaT_tuple_new
> lua: don't raise a Lua error from luaT_tuple_new()
> module api/lua: add luaT_tuple_encode()
> module api/lua: expose luaT_tuple_new()
> module api/lua: add API_EXPORT to tuple functions
> module api: add API_EXPORT to key_def functions
> module api: add box_key_def_new_v2()
> module api: add box_key_def_dump_parts()
> module api: expose box_key_def_validate_tuple()
> module api: expose box_key_def_merge()
> module api: expose box_key_def_extract_key()
> module api: add box_key_def_validate_key()
> module api: add box_key_def_validate_full_key()
>
> src/CMakeLists.txt | 2 +-
> src/box/index.h | 5 +-
> src/box/key_def.c | 264 +++++
> src/box/key_def.h | 268 ++++-
> src/box/lua/tuple.c | 210 +++-
> src/box/lua/tuple.h | 46 +-
> src/exports.h | 15 +
> src/lib/core/fiber.c | 24 +
> src/lib/core/fiber.h | 76 ++
> src/lua/utils.c | 6 +
> src/lua/utils.h | 20 +
> test/app-tap/CMakeLists.txt | 6 +
> test/app-tap/module_api.c | 1711 ++++++++++++++++++++++++++++++
> test/app-tap/module_api.test.lua | 63 +-
> test/unit/luaT_tuple_new.c | 42 +-
> test/unit/luaT_tuple_new.result | 6 +-
> 16 files changed, 2705 insertions(+), 59 deletions(-)
>
> --
> 2.25.0
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module
2020-10-15 13:19 ` Alexander Turenko
@ 2020-10-16 6:05 ` Alexander Turenko
0 siblings, 0 replies; 33+ messages in thread
From: Alexander Turenko @ 2020-10-16 6:05 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Thu, Oct 15, 2020 at 04:19:58PM +0300, Alexander Turenko wrote:
> Updated.
>
> Changes from v3 to the final version (I hope it is final):
>
> - Fixed sorting of the box region functions in src/exports.h.
> - Added forgotten API_EXPORT for box_region_truncate().
> - Added a comment for luaT_tuple_encode_table() reference in the Lua registry.
> - Added the <size_t *key_size_ptr> parameter to box_key_def_validate_key() and
> box_key_def_validate_full_key().
> - Fixed API comments for key_def functions with mention of the diagnostics
> area.
> - Set a memory error to the diagnostics area for box_region_*alloc() functions.
List of changes that I made before pushing:
- test_iscdata(): fixed negative index testing:
| - lua_pushboolean(L, res == exp);
| + lua_pushboolean(L, ok);
- Found and fixed 'unused variable' warnings on module_api.c on
RelWithDebInfo build.
Pushed to master, 2.5, 2.4 and 1.10.
Many thanks to Vlad for help with designing, eagle eyes and fast
responses.
WBR, Alexander Turenko.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module
2020-10-12 23:23 [Tarantool-patches] [PATCH v3 00/16] module api: extend for external key_def Lua module Alexander Turenko
` (17 preceding siblings ...)
2020-10-15 13:19 ` Alexander Turenko
@ 2020-10-15 20:12 ` Vladislav Shpilevoy
18 siblings, 0 replies; 33+ messages in thread
From: Vladislav Shpilevoy @ 2020-10-15 20:12 UTC (permalink / raw)
To: Alexander Turenko; +Cc: tarantool-patches
Hi! Thanks for the patchset!
LGTM.
^ permalink raw reply [flat|nested] 33+ messages in thread