* [tarantool-patches] [PATCH v1 1/1] lua: export key_def methods for LUA key_def object @ 2019-03-25 12:27 Kirill Shcherbatov 2019-03-26 1:46 ` [tarantool-patches] " Alexander Turenko 2019-04-23 15:05 ` Konstantin Osipov 0 siblings, 2 replies; 7+ messages in thread From: Kirill Shcherbatov @ 2019-03-25 12:27 UTC (permalink / raw) To: tarantool-patches, alexander.turenko; +Cc: Kirill Shcherbatov Functions extract_key, compare, compare_with_key, merge defined for key_def introduced for LUA key_def object. Close #4025 @TarantoolBot document Title: Built-in methods for key_def object Now it is possible to work with index base object - key definition in LUA. Available methods: key = key_def:extract_key(tuple) key_def:compare(tuple_a, tuple_b) key_def:compare_with_key(tuple, key), where key is tuple or table new_key_def = key_def:merge(second_key_def) Example: -- Removal values for secondary non-unique index key_def = require('key_def') s = box.schema.space.create('test') pk = s:create_index('pk') idx = s:create_index('test', {unique=false, parts = {{2, 'number', path = 'a'}, {2, 'number', path = 'b'}}}) s:insert{1, {a = 1, b = 1}} s:insert{2, {a = 1, b = 2}} pk_def = key_def.new(pk.parts) for _, tuple in pairs(idx:select({1, nil})) do local key = pk_def:extract_key(tuple) pk:delete(key) end --- src/box/CMakeLists.txt | 1 + src/box/lua/init.c | 2 + src/box/lua/key_def.c | 134 ++++++++++++++++++++++++++++++++++++++++ src/box/lua/key_def.h | 8 +++ src/box/lua/key_def.lua | 19 ++++++ src/box/lua/space.cc | 28 +-------- test/box/tuple.result | 133 +++++++++++++++++++++++++++++++++++++++ test/box/tuple.test.lua | 39 ++++++++++++ 8 files changed, 338 insertions(+), 26 deletions(-) create mode 100644 src/box/lua/key_def.lua diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt index f25c21045..906ce22e6 100644 --- a/src/box/CMakeLists.txt +++ b/src/box/CMakeLists.txt @@ -12,6 +12,7 @@ lua_source(lua_sources lua/net_box.lua) lua_source(lua_sources lua/upgrade.lua) lua_source(lua_sources lua/console.lua) lua_source(lua_sources lua/xlog.lua) +lua_source(lua_sources lua/key_def.lua) set(bin_sources) bin_source(bin_sources bootstrap.snap bootstrap.h) diff --git a/src/box/lua/init.c b/src/box/lua/init.c index 69f346414..68ef58909 100644 --- a/src/box/lua/init.c +++ b/src/box/lua/init.c @@ -63,6 +63,7 @@ extern char session_lua[], tuple_lua[], + key_def_lua[], schema_lua[], load_cfg_lua[], xlog_lua[], @@ -81,6 +82,7 @@ static const char *lua_sources[] = { "box/console", console_lua, "box/load_cfg", load_cfg_lua, "box/xlog", xlog_lua, + "box/key_def", key_def_lua, NULL }; diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c index 813582913..6d2c06afb 100644 --- a/src/box/lua/key_def.c +++ b/src/box/lua/key_def.c @@ -34,8 +34,10 @@ #include <lua.h> #include <lauxlib.h> #include "diag.h" +#include "tuple.h" #include "box/key_def.h" #include "box/box.h" +#include "box/tuple.h" #include "box/coll_id_cache.h" #include "lua/utils.h" #include "box/tuple_format.h" /* TUPLE_INDEX_BASE */ @@ -149,6 +151,28 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part) return 0; } +void +lbox_fill_key_part(struct lua_State *L, const struct key_part *part) +{ + lua_newtable(L); + lua_pushstring(L, field_type_strs[part->type]); + lua_setfield(L, -2, "type"); + lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE); + lua_setfield(L, -2, "fieldno"); + if (part->path != NULL) { + lua_pushlstring(L, part->path, part->path_len); + lua_setfield(L, -2, "path"); + } + lua_pushboolean(L, key_part_is_nullable(part)); + lua_setfield(L, -2, "is_nullable"); + if (part->coll_id != COLL_NONE) { + struct coll_id *coll_id = coll_by_id(part->coll_id); + assert(coll_id != NULL); + lua_pushstring(L, coll_id->name); + lua_setfield(L, -2, "collation"); + } +} + struct key_def * check_key_def(struct lua_State *L, int idx) { @@ -175,6 +199,103 @@ lbox_key_def_gc(struct lua_State *L) return 0; } +static int +lbox_key_def_extract_key(lua_State *L) +{ + struct key_def *key_def; + struct tuple *tuple; + if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL || + (tuple = luaT_istuple(L, 2)) == NULL) + return luaL_error(L, "Usage key_def:extract_key(tuple)"); + + uint32_t key_size; + char *key = tuple_extract_key(tuple, key_def, &key_size); + if (key == NULL) + return luaT_error(L); + + struct tuple *ret = + box_tuple_new(box_tuple_format_default(), key, key + key_size); + if (ret == NULL) + return luaT_error(L); + luaT_pushtuple(L, ret); + return 1; +} + +static int +lbox_key_def_compare(lua_State *L) +{ + struct key_def *key_def; + struct tuple *tuple_a, *tuple_b; + if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL || + (tuple_a = luaT_istuple(L, 2)) == NULL || + (tuple_b = luaT_istuple(L, 3)) == NULL) + return luaL_error(L, "Usage key_def:compare(tuple_a, tuple_b)"); + + int rc = tuple_compare(tuple_a, tuple_b, key_def); + lua_pushinteger(L, rc); + return 1; +} + +static int +lbox_key_def_compare_with_key(lua_State *L) +{ + struct key_def *key_def; + struct tuple *tuple, *key_tuple; + if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL || + (tuple = luaT_istuple(L, 2)) == NULL) + goto usage_error; + + lua_remove(L, 1); + lua_remove(L, 1); + if (luaT_tuple_new(L, box_tuple_format_default()) != 1 || + (key_tuple = luaT_istuple(L, -1)) == NULL) + goto usage_error; + + const char *key = tuple_data(key_tuple); + assert(mp_typeof(*key) == MP_ARRAY); + uint32_t part_count = mp_decode_array(&key); + + int rc = tuple_compare_with_key(tuple, key, part_count, key_def); + lua_pushinteger(L, rc); + return 1; +usage_error: + return luaL_error(L, "Usage key_def:compare_with_key(tuple, key)"); +} + +static int +lbox_key_def_merge(lua_State *L) +{ + struct key_def *key_def_a, *key_def_b; + if (lua_gettop(L) != 2 || (key_def_a = check_key_def(L, 1)) == NULL || + (key_def_b = check_key_def(L, 2)) == NULL) + return luaL_error(L, "Usage key_def:merge(second_key_def)"); + + struct key_def *new_key_def = key_def_merge(key_def_a, key_def_b); + if (new_key_def == NULL) + return luaT_error(L); + + *(struct key_def **)luaL_pushcdata(L, key_def_type_id) = new_key_def; + lua_pushcfunction(L, lbox_key_def_gc); + luaL_setcdatagc(L, -2); + return 1; +} + +static int +lbox_key_def_to_map(struct lua_State *L) +{ + struct key_def *key_def; + if (lua_gettop(L) != 1 || (key_def = check_key_def(L, 1)) == NULL) + return luaL_error(L, "Usage key_def:tomap()"); + + lua_createtable(L, key_def->part_count, 0); + for (uint32_t i = 0; i < key_def->part_count; ++i) { + lua_pushnumber(L, i + 1); + lbox_fill_key_part(L, &key_def->parts[i]); + lua_settable(L, -3); + } + return 1; +} + /** * Create a new key_def from a Lua table. * @@ -236,5 +357,18 @@ luaopen_key_def(struct lua_State *L) }; luaL_register_module(L, "key_def", meta); + lua_newtable(L); /* key_def.internal */ + lua_pushcfunction(L, lbox_key_def_extract_key); + lua_setfield(L, -2, "extract_key"); + lua_pushcfunction(L, lbox_key_def_compare); + lua_setfield(L, -2, "compare"); + lua_pushcfunction(L, lbox_key_def_compare_with_key); + lua_setfield(L, -2, "compare_with_key"); + lua_pushcfunction(L, lbox_key_def_merge); + lua_setfield(L, -2, "merge"); + lua_pushcfunction(L, lbox_key_def_to_map); + lua_setfield(L, -2, "tomap"); + lua_setfield(L, -2, "internal"); + return 1; } diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h index 61894d452..02217d3a3 100644 --- a/src/box/lua/key_def.h +++ b/src/box/lua/key_def.h @@ -36,6 +36,14 @@ extern "C" { #endif /* defined(__cplusplus) */ struct lua_State; +struct key_part; + +/** + * Prepare a new table representing a key_part on top of the Lua + * stack. + */ +void +lbox_fill_key_part(struct lua_State *L, const struct key_part *part); /** * Extract a key_def object from a Lua stack. diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua new file mode 100644 index 000000000..5dc8d9357 --- /dev/null +++ b/src/box/lua/key_def.lua @@ -0,0 +1,19 @@ +local ffi = require('ffi') +local key_def = require('key_def') +local key_def_t = ffi.typeof('struct key_def') + +local methods = { + ['extract_key'] = key_def.internal.extract_key, + ['compare'] = key_def.internal.compare, + ['compare_with_key'] = key_def.internal.compare_with_key, + ['merge'] = key_def.internal.merge, + ['tomap'] = key_def.internal.tomap, + ['__serialize'] = key_def.internal.tomap, +} + +ffi.metatype(key_def_t, { + __index = function(self, key) + return methods[key] + end, + __tostring = function(key_def) return "<struct key_def&>" end, +}) diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc index 9dfc97b6a..9be27959c 100644 --- a/src/box/lua/space.cc +++ b/src/box/lua/space.cc @@ -30,6 +30,7 @@ */ #include "box/lua/space.h" #include "box/lua/tuple.h" +#include "box/lua/key_def.h" #include "box/sql/sqlLimit.h" #include "lua/utils.h" #include "lua/trigger.h" @@ -286,32 +287,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i) for (uint32_t j = 0; j < index_def->key_def->part_count; j++) { lua_pushnumber(L, j + 1); - lua_newtable(L); - const struct key_part *part = - &index_def->key_def->parts[j]; - - lua_pushstring(L, field_type_strs[part->type]); - lua_setfield(L, -2, "type"); - - lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE); - lua_setfield(L, -2, "fieldno"); - - if (part->path != NULL) { - lua_pushlstring(L, part->path, part->path_len); - lua_setfield(L, -2, "path"); - } - - lua_pushboolean(L, key_part_is_nullable(part)); - lua_setfield(L, -2, "is_nullable"); - - if (part->coll_id != COLL_NONE) { - struct coll_id *coll_id = - coll_by_id(part->coll_id); - assert(coll_id != NULL); - lua_pushstring(L, coll_id->name); - lua_setfield(L, -2, "collation"); - } - + lbox_fill_key_part(L, &index_def->key_def->parts[j]); lua_settable(L, -3); /* index[k].parts[j] */ } diff --git a/test/box/tuple.result b/test/box/tuple.result index 82ad8404d..a0209ca4a 100644 --- a/test/box/tuple.result +++ b/test/box/tuple.result @@ -1231,3 +1231,136 @@ s2:frommap({a="1", k="11"}):tomap({names_only = true}) s2:drop() --- ... +-- +-- gh-4025: Introduce built-in function to get index key +-- from tuple. +-- +key_def = require('key_def') +--- +... +s = box.schema.space.create('test', {engine='vinyl'}) +--- +... +pk = s:create_index('pk') +--- +... +sk = s:create_index('sk', {unique=false, parts = {{2, 'number'}, {3, 'number'}}}) +--- +... +tuple_a = box.tuple.new({1, 1, 22}) +--- +... +tuple_b = box.tuple.new({2, 1, 11}) +--- +... +tuple_c = box.tuple.new({3, 1, 22}) +--- +... +pk_def = key_def.new(pk.parts) +--- +... +pk_def:extract_key(tuple_a) +--- +- [1] +... +sk_def = key_def.new(sk.parts) +--- +... +sk_def:extract_key(tuple_a) +--- +- [1, 22] +... +s:insert(tuple_a) +--- +- [1, 1, 22] +... +s:insert(tuple_b) +--- +- [2, 1, 11] +... +for _, tuple in pairs(sk:select({1, nil})) do pk:delete(pk_def:extract_key(tuple)) end +--- +... +s:select() +--- +- [] +... +pk_def:compare(tuple_b, tuple_a) +--- +- 1 +... +pk_def:compare(tuple_b, tuple_c) +--- +- -1 +... +sk_def:compare(tuple_b, tuple_a) +--- +- -1 +... +sk_def:compare(tuple_a, tuple_c) +--- +- 0 +... +pk_sk_def = pk_def:merge(sk_def) +--- +... +pk_sk_def:extract_key(tuple_a) +--- +- [1, 1, 22] +... +pk_sk_def:compare(tuple_a, tuple_b) +--- +- -1 +... +sk_pk_def = sk_def:merge(pk_def) +--- +... +sk_pk_def:extract_key(tuple_a) +--- +- [1, 22, 1] +... +sk_pk_def:compare(tuple_a, tuple_b) +--- +- 1 +... +key = sk_def:extract_key(tuple_a) +--- +... +sk_def:compare_with_key(tuple_a, key) +--- +- 0 +... +sk_def:compare_with_key(tuple_a, {1, 20}) +--- +- 1 +... +sk_def:tomap() +--- +- - type: number + is_nullable: false + fieldno: 2 + - type: number + is_nullable: false + fieldno: 3 +... +sk_def +--- +- - type: number + is_nullable: false + fieldno: 2 + - type: number + is_nullable: false + fieldno: 3 +... +print(sk_def) +--- +... +key_def.new(sk_def:tomap()) +--- +- - type: number + is_nullable: false + fieldno: 2 + - type: number + is_nullable: false + fieldno: 3 +... diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua index 8030b0884..0a626b36a 100644 --- a/test/box/tuple.test.lua +++ b/test/box/tuple.test.lua @@ -416,3 +416,42 @@ s2:frommap({a="1", k="11"}) s2:frommap({a="1", k="11"}):tomap({names_only = true}) s2:drop() + +-- +-- gh-4025: Introduce built-in function to get index key +-- from tuple. +-- +key_def = require('key_def') +s = box.schema.space.create('test', {engine='vinyl'}) +pk = s:create_index('pk') +sk = s:create_index('sk', {unique=false, parts = {{2, 'number'}, {3, 'number'}}}) +tuple_a = box.tuple.new({1, 1, 22}) +tuple_b = box.tuple.new({2, 1, 11}) +tuple_c = box.tuple.new({3, 1, 22}) +pk_def = key_def.new(pk.parts) +pk_def:extract_key(tuple_a) +sk_def = key_def.new(sk.parts) +sk_def:extract_key(tuple_a) +s:insert(tuple_a) +s:insert(tuple_b) +for _, tuple in pairs(sk:select({1, nil})) do pk:delete(pk_def:extract_key(tuple)) end +s:select() +pk_def:compare(tuple_b, tuple_a) +pk_def:compare(tuple_b, tuple_c) +sk_def:compare(tuple_b, tuple_a) +sk_def:compare(tuple_a, tuple_c) + +pk_sk_def = pk_def:merge(sk_def) +pk_sk_def:extract_key(tuple_a) +pk_sk_def:compare(tuple_a, tuple_b) +sk_pk_def = sk_def:merge(pk_def) +sk_pk_def:extract_key(tuple_a) +sk_pk_def:compare(tuple_a, tuple_b) + +key = sk_def:extract_key(tuple_a) +sk_def:compare_with_key(tuple_a, key) +sk_def:compare_with_key(tuple_a, {1, 20}) +sk_def:tomap() +sk_def +print(sk_def) +key_def.new(sk_def:tomap()) -- 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object 2019-03-25 12:27 [tarantool-patches] [PATCH v1 1/1] lua: export key_def methods for LUA key_def object Kirill Shcherbatov @ 2019-03-26 1:46 ` Alexander Turenko 2019-03-26 13:37 ` Kirill Shcherbatov 2019-04-23 15:05 ` Konstantin Osipov 1 sibling, 1 reply; 7+ messages in thread From: Alexander Turenko @ 2019-03-26 1:46 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: tarantool-patches You don't include the fixup commit, so I'll paste it here and comment: > diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c > index 36c469526..f819bfed9 100644 > --- a/src/box/lua/key_def.c > +++ b/src/box/lua/key_def.c > @@ -33,6 +33,7 @@ > > #include <lua.h> > #include <lauxlib.h> > +#include "fiber.h" > #include "diag.h" > #include "box/key_def.h" > #include "box/box.h" > @@ -94,11 +95,11 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part) > /* Set part->is_nullable and part->nullable_action. */ > lua_pushstring(L, "is_nullable"); > lua_gettable(L, -2); > - if (lua_isnil(L, -1)) { > + if (lua_isnil(L, -1) || lua_toboolean(L, -1) == false) { lua_toboolean() returns int, so it would be better to compare it with 0 explicitly. BTW, nice catch. > part->is_nullable = false; > part->nullable_action = ON_CONFLICT_ACTION_DEFAULT; > } else { > - part->is_nullable = lua_toboolean(L, -1); > + part->is_nullable = true; > part->nullable_action = ON_CONFLICT_ACTION_NONE; > } > lua_pop(L, 1); > @@ -143,9 +144,29 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part) > /* Set part->sort_order. */ > part->sort_order = SORT_ORDER_ASC; > > - /* Set part->path. */ > - part->path = NULL; > - > + /* Set part->path for JSON index. */ > + lua_pushstring(L, "path"); > + lua_gettable(L, -2); > + if (!lua_isnil(L, -1)) { > + size_t path_len; > + const char *path = lua_tolstring(L, -1, &path_len); > + if (json_path_validate(path, path_len, TUPLE_INDEX_BASE) != 0) { > + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, > + 0 /* FIXME */ + TUPLE_INDEX_BASE, "invalid path"); FIXME? > + return -1; > + } > + char *tmp = region_alloc(&fiber()->gc, path_len + 1); Should we do region_used() in lbox_key_def_new() before the loop and do region_truncate() after? Even more, I guess we can don't copy strings here at all: lua will not collect them until we'll return from key_def.new() and it is enough. > + if (tmp == NULL) { > + diag_set(OutOfMemory, path_len + 1, "region", "path"); > + return -1; > + } > + memcpy(tmp, path, path_len); > + tmp[path_len] = '\0'; > + part->path = tmp; > + } else { > + part->path = NULL; > + } > + lua_pop(L, 1); > return 0; > } On Mon, Mar 25, 2019 at 03:27:41PM +0300, Kirill Shcherbatov wrote: > Functions extract_key, compare, compare_with_key, merge defined > for key_def introduced for LUA key_def object. > > Close #4025 See also a list of issues from mine patch, they are closed with this patchset too. > +void > +lbox_fill_key_part(struct lua_State *L, const struct key_part *part) Such functions are usually named lbox_pushfoo(), in this case I propose lbox_push_key_part(). > +static int > +lbox_key_def_extract_key(lua_State *L) > +{ > + struct key_def *key_def; > + struct tuple *tuple; > + if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL || > + (tuple = luaT_istuple(L, 2)) == NULL) > + return luaL_error(L, "Usage key_def:extract_key(tuple)"); Nit: Add a colon after 'Usage' (here and below). > +static int > +lbox_key_def_compare_with_key(lua_State *L) > +{ > + struct key_def *key_def; > + struct tuple *tuple, *key_tuple; > + if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL || > + (tuple = luaT_istuple(L, 2)) == NULL) > + goto usage_error; > + > + lua_remove(L, 1); > + lua_remove(L, 1); > + if (luaT_tuple_new(L, box_tuple_format_default()) != 1 || Maybe extract https://github.com/tarantool/tarantool/commit/6427413d27dad7cdaaff46a5705ad45b93549275 too and use it here (it is part of Totktonada/gh-3276-on-board-merger patchset? > +static int > +lbox_key_def_merge(lua_State *L) > +{ > + struct key_def *key_def_a, *key_def_b; > + if (lua_gettop(L) != 2 || (key_def_a = check_key_def(L, 1)) == NULL || > + (key_def_b = check_key_def(L, 2)) == NULL) > + return luaL_error(L, "Usage key_def:merge(second_key_def)"); > + > + struct key_def *new_key_def = key_def_merge(key_def_a, key_def_b); > + if (new_key_def == NULL) > + return luaT_error(L); > + > + *(struct key_def **)luaL_pushcdata(L, key_def_type_id) = new_key_def; Nit: add a whitespace after cast. > + lua_pushcfunction(L, lbox_key_def_gc); > + luaL_setcdatagc(L, -2); > + return 1; > +} > + > +static int > +lbox_key_def_to_map(struct lua_State *L) Why tomap? It produce an array as result. Maybe lbox_key_def_to_table()? Or maybe just lbox_key_def_serialize() like lbox_fiber_serialize() (and don't expose it as :tomap() / :totable()). > + lua_createtable(L, key_def->part_count, 0); > + for (uint32_t i = 0; i < key_def->part_count; ++i) { > + lua_pushnumber(L, i + 1); > + lbox_fill_key_part(L, &key_def->parts[i]); > + lua_settable(L, -3); lua_rawgei() is more convenient here. > +/** > + * Prepare a new table representing a key_part on top of the Lua > + * stack. > + */ I would rephrase a bit: > Push a new table representing a key_part to a Lua stack. > +ffi.metatype(key_def_t, { > + __index = function(self, key) > + return methods[key] > + end, > + __tostring = function(key_def) return "<struct key_def&>" end, Nit: add a whitespace before the ampersand. > diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua > index 8030b0884..0a626b36a 100644 > --- a/test/box/tuple.test.lua > +++ b/test/box/tuple.test.lua There is test/box-tap/key_def.test.lua (introduced in the first path). > @@ -416,3 +416,42 @@ s2:frommap({a="1", k="11"}) > s2:frommap({a="1", k="11"}):tomap({names_only = true}) > > s2:drop() > + > +-- > +-- gh-4025: Introduce built-in function to get index key > +-- from tuple. > +-- > +key_def = require('key_def') > +s = box.schema.space.create('test', {engine='vinyl'}) Why vinyl? I don't think we even need to create a space for this test. > +pk = s:create_index('pk') > +sk = s:create_index('sk', {unique=false, parts = {{2, 'number'}, {3, 'number'}}}) > +tuple_a = box.tuple.new({1, 1, 22}) > +tuple_b = box.tuple.new({2, 1, 11}) > +tuple_c = box.tuple.new({3, 1, 22}) > +pk_def = key_def.new(pk.parts) > +pk_def:extract_key(tuple_a) > +sk_def = key_def.new(sk.parts) > +sk_def:extract_key(tuple_a) > +s:insert(tuple_a) > +s:insert(tuple_b) > +for _, tuple in pairs(sk:select({1, nil})) do pk:delete(pk_def:extract_key(tuple)) end > +s:select() > +pk_def:compare(tuple_b, tuple_a) > +pk_def:compare(tuple_b, tuple_c) > +sk_def:compare(tuple_b, tuple_a) > +sk_def:compare(tuple_a, tuple_c) > + > +pk_sk_def = pk_def:merge(sk_def) > +pk_sk_def:extract_key(tuple_a) > +pk_sk_def:compare(tuple_a, tuple_b) > +sk_pk_def = sk_def:merge(pk_def) > +sk_pk_def:extract_key(tuple_a) > +sk_pk_def:compare(tuple_a, tuple_b) > + > +key = sk_def:extract_key(tuple_a) > +sk_def:compare_with_key(tuple_a, key) > +sk_def:compare_with_key(tuple_a, {1, 20}) > +sk_def:tomap() > +sk_def > +print(sk_def) > +key_def.new(sk_def:tomap()) > -- > 2.21.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object 2019-03-26 1:46 ` [tarantool-patches] " Alexander Turenko @ 2019-03-26 13:37 ` Kirill Shcherbatov 2019-03-26 22:54 ` Alexander Turenko 0 siblings, 1 reply; 7+ messages in thread From: Kirill Shcherbatov @ 2019-03-26 13:37 UTC (permalink / raw) To: tarantool-patches, Alexander Turenko Hi! Thank you for review. https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110 Pathes for 2.1.1 Fixup for your commit: ============================================================= --- src/box/lua/key_def.c | 48 +++++++++++++++++++++++++++-------- test/box-tap/key_def.test.lua | 11 ++++++++ 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c index 36c469526..38a9e80a7 100644 --- a/src/box/lua/key_def.c +++ b/src/box/lua/key_def.c @@ -33,6 +33,7 @@ #include <lua.h> #include <lauxlib.h> +#include "fiber.h" #include "diag.h" #include "box/key_def.h" #include "box/box.h" @@ -48,8 +49,10 @@ static uint32_t key_def_type_id = 0; * When successful return 0, otherwise return -1 and set a diag. */ static int -luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part) +luaT_key_def_set_part(struct lua_State *L, struct key_part_def *parts, + int part_idx, struct region *region) { + struct key_part_def *part = &parts[part_idx]; /* Set part->fieldno. */ lua_pushstring(L, "fieldno"); lua_gettable(L, -2); @@ -94,11 +97,11 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part) /* Set part->is_nullable and part->nullable_action. */ lua_pushstring(L, "is_nullable"); lua_gettable(L, -2); - if (lua_isnil(L, -1)) { + if (lua_isnil(L, -1) || lua_toboolean(L, -1) == 0) { part->is_nullable = false; part->nullable_action = ON_CONFLICT_ACTION_DEFAULT; } else { - part->is_nullable = lua_toboolean(L, -1); + part->is_nullable = true; part->nullable_action = ON_CONFLICT_ACTION_NONE; } lua_pop(L, 1); @@ -143,9 +146,29 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *part) /* Set part->sort_order. */ part->sort_order = SORT_ORDER_ASC; - /* Set part->path. */ - part->path = NULL; - + /* Set part->path for JSON index. */ + lua_pushstring(L, "path"); + lua_gettable(L, -2); + if (!lua_isnil(L, -1)) { + size_t path_len; + const char *path = lua_tolstring(L, -1, &path_len); + if (json_path_validate(path, path_len, TUPLE_INDEX_BASE) != 0) { + diag_set(ClientError, ER_WRONG_INDEX_OPTIONS, + part_idx + TUPLE_INDEX_BASE, "invalid path"); + return -1; + } + char *tmp = region_alloc(region, path_len + 1); + if (tmp == NULL) { + diag_set(OutOfMemory, path_len + 1, "region", "path"); + return -1; + } + memcpy(tmp, path, path_len); + tmp[path_len] = '\0'; + part->path = tmp; + } else { + part->path = NULL; + } + lua_pop(L, 1); return 0; } @@ -191,28 +214,31 @@ lbox_key_def_new(struct lua_State *L) return luaL_error(L, "Bad params, use: key_def.new({" "{fieldno = fieldno, type = type" "[, is_nullable = <boolean>]" + "[, path = <string>]" "[, collation_id = <number>]" "[, collation = <string>]}, ...}"); uint32_t part_count = lua_objlen(L, 1); const ssize_t parts_size = sizeof(struct key_part_def) * part_count; - struct key_part_def *parts = malloc(parts_size); + struct region *region = &fiber()->gc; + size_t region_svp = region_used(region); + struct key_part_def *parts = region_alloc(region, parts_size); if (parts == NULL) { - diag_set(OutOfMemory, parts_size, "malloc", "parts"); + diag_set(OutOfMemory, parts_size, "region", "parts"); return luaT_error(L); } for (uint32_t i = 0; i < part_count; ++i) { lua_pushinteger(L, i + 1); lua_gettable(L, 1); - if (luaT_key_def_set_part(L, &parts[i]) != 0) { - free(parts); + if (luaT_key_def_set_part(L, parts, i, region) != 0) { + region_truncate(region, region_svp); return luaT_error(L); } } struct key_def *key_def = key_def_new(parts, part_count); - free(parts); + region_truncate(region, region_svp); if (key_def == NULL) return luaT_error(L); diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua index 7e6e0e330..fd5d60b18 100755 --- a/test/box-tap/key_def.test.lua +++ b/test/box-tap/key_def.test.lua @@ -7,6 +7,7 @@ local key_def = require('key_def') local usage_error = 'Bad params, use: key_def.new({' .. '{fieldno = fieldno, type = type' .. '[, is_nullable = <boolean>]' .. + '[, path = <string>]' .. '[, collation_id = <number>]' .. '[, collation = <string>]}, ...}' @@ -95,6 +96,15 @@ local cases = { params = {{}, {}}, exp_err = usage_error, }, + { + 'Invalid JSON path', + parts = {{ + fieldno = 1, + type = 'string', + path = "[3[", + }}, + exp_err = "Wrong index options (field 1): invalid path", + }, { 'Success case; zero parts', parts = {}, @@ -105,6 +115,7 @@ local cases = { parts = { fieldno = 1, type = 'string', + path = "[3]", }, exp_err = nil, }, -- 2.21.0 ============================================================= Main patch: ============================================================= Functions extract_key, compare, compare_with_key, merge defined for key_def introduced for LUA key_def object. Closes #4025 Closes #3398 @TarantoolBot document Title: Built-in methods for key_def object Now it is possible to work with index base object - key definition in LUA. Available methods: key = key_def:extract_key(tuple) key_def:compare(tuple_a, tuple_b) key_def:compare_with_key(tuple, key), where key is tuple or table new_key_def = key_def:merge(second_key_def) Example: -- Removal values for secondary non-unique index key_def = require('key_def') s = box.schema.space.create('test') pk = s:create_index('pk') idx = s:create_index('test', {unique=false, parts = {{2, 'number', path = 'a'}, {2, 'number', path = 'b'}}}) s:insert{1, {a = 1, b = 1}} s:insert{2, {a = 1, b = 2}} pk_def = key_def.new(pk.parts) for _, tuple in pairs(idx:select({1, nil})) do local key = pk_def:extract_key(tuple) pk:delete(key) end --- src/box/CMakeLists.txt | 1 + src/box/lua/init.c | 2 + src/box/lua/key_def.c | 133 ++++++++++++++++++++++++++++++++++ src/box/lua/key_def.h | 7 ++ src/box/lua/key_def.lua | 19 +++++ src/box/lua/space.cc | 28 +------ test/box-tap/key_def.test.lua | 43 ++++++++++- 7 files changed, 206 insertions(+), 27 deletions(-) create mode 100644 src/box/lua/key_def.lua diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt index f25c21045..906ce22e6 100644 --- a/src/box/CMakeLists.txt +++ b/src/box/CMakeLists.txt @@ -12,6 +12,7 @@ lua_source(lua_sources lua/net_box.lua) lua_source(lua_sources lua/upgrade.lua) lua_source(lua_sources lua/console.lua) lua_source(lua_sources lua/xlog.lua) +lua_source(lua_sources lua/key_def.lua) set(bin_sources) bin_source(bin_sources bootstrap.snap bootstrap.h) diff --git a/src/box/lua/init.c b/src/box/lua/init.c index 69f346414..68ef58909 100644 --- a/src/box/lua/init.c +++ b/src/box/lua/init.c @@ -63,6 +63,7 @@ extern char session_lua[], tuple_lua[], + key_def_lua[], schema_lua[], load_cfg_lua[], xlog_lua[], @@ -81,6 +82,7 @@ static const char *lua_sources[] = { "box/console", console_lua, "box/load_cfg", load_cfg_lua, "box/xlog", xlog_lua, + "box/key_def", key_def_lua, NULL }; diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c index 38a9e80a7..c2eb3c071 100644 --- a/src/box/lua/key_def.c +++ b/src/box/lua/key_def.c @@ -35,8 +35,10 @@ #include <lauxlib.h> #include "fiber.h" #include "diag.h" +#include "tuple.h" #include "box/key_def.h" #include "box/box.h" +#include "box/tuple.h" #include "box/coll_id_cache.h" #include "lua/utils.h" #include "box/tuple_format.h" /* TUPLE_INDEX_BASE */ @@ -172,6 +174,28 @@ luaT_key_def_set_part(struct lua_State *L, struct key_part_def *parts, return 0; } +void +lbox_push_key_part(struct lua_State *L, const struct key_part *part) +{ + lua_newtable(L); + lua_pushstring(L, field_type_strs[part->type]); + lua_setfield(L, -2, "type"); + lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE); + lua_setfield(L, -2, "fieldno"); + if (part->path != NULL) { + lua_pushlstring(L, part->path, part->path_len); + lua_setfield(L, -2, "path"); + } + lua_pushboolean(L, key_part_is_nullable(part)); + lua_setfield(L, -2, "is_nullable"); + if (part->coll_id != COLL_NONE) { + struct coll_id *coll_id = coll_by_id(part->coll_id); + assert(coll_id != NULL); + lua_pushstring(L, coll_id->name); + lua_setfield(L, -2, "collation"); + } +} + struct key_def * check_key_def(struct lua_State *L, int idx) { @@ -198,6 +222,102 @@ lbox_key_def_gc(struct lua_State *L) return 0; } +static int +lbox_key_def_extract_key(lua_State *L) +{ + struct key_def *key_def; + struct tuple *tuple; + if (lua_gettop(L) != 2 || (key_def = check_key_def(L, 1)) == NULL || + (tuple = luaT_istuple(L, 2)) == NULL) + return luaL_error(L, "Usage: key_def:extract_key(tuple)"); + + uint32_t key_size; + char *key = tuple_extract_key(tuple, key_def, &key_size); + if (key == NULL) + return luaT_error(L); + + struct tuple *ret = + box_tuple_new(box_tuple_format_default(), key, key + key_size); + if (ret == NULL) + return luaT_error(L); + luaT_pushtuple(L, ret); + return 1; +} + +static int +lbox_key_def_compare(lua_State *L) +{ + struct key_def *key_def; + struct tuple *tuple_a, *tuple_b; + if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL || + (tuple_a = luaT_istuple(L, 2)) == NULL || + (tuple_b = luaT_istuple(L, 3)) == NULL) + return luaL_error(L, "Usage: key_def:compare(tuple_a, tuple_b)"); + + int rc = tuple_compare(tuple_a, tuple_b, key_def); + lua_pushinteger(L, rc); + return 1; +} + +static int +lbox_key_def_compare_with_key(lua_State *L) +{ + struct key_def *key_def; + struct tuple *tuple, *key_tuple; + if (lua_gettop(L) != 3 || (key_def = check_key_def(L, 1)) == NULL || + (tuple = luaT_istuple(L, 2)) == NULL) + goto usage_error; + + lua_remove(L, 1); + lua_remove(L, 1); + if (luaT_tuple_new(L, box_tuple_format_default()) != 1 || + (key_tuple = luaT_istuple(L, -1)) == NULL) + goto usage_error; + + const char *key = tuple_data(key_tuple); + assert(mp_typeof(*key) == MP_ARRAY); + uint32_t part_count = mp_decode_array(&key); + + int rc = tuple_compare_with_key(tuple, key, part_count, key_def); + lua_pushinteger(L, rc); + return 1; +usage_error: + return luaL_error(L, "Usage: key_def:compare_with_key(tuple, key)"); +} + +static int +lbox_key_def_merge(lua_State *L) +{ + struct key_def *key_def_a, *key_def_b; + if (lua_gettop(L) != 2 || (key_def_a = check_key_def(L, 1)) == NULL || + (key_def_b = check_key_def(L, 2)) == NULL) + return luaL_error(L, "Usage: key_def:merge(second_key_def)"); + + struct key_def *new_key_def = key_def_merge(key_def_a, key_def_b); + if (new_key_def == NULL) + return luaT_error(L); + + *(struct key_def **) luaL_pushcdata(L, key_def_type_id) = new_key_def; + lua_pushcfunction(L, lbox_key_def_gc); + luaL_setcdatagc(L, -2); + return 1; +} + +static int +lbox_key_def_to_table(struct lua_State *L) +{ + struct key_def *key_def; + if (lua_gettop(L) != 1 || (key_def = check_key_def(L, 1)) == NULL) + return luaL_error(L, "Usage: key_def:to_table()"); + + lua_createtable(L, key_def->part_count, 0); + for (uint32_t i = 0; i < key_def->part_count; ++i) { + lbox_push_key_part(L, &key_def->parts[i]); + lua_rawseti(L, -2, i + 1); + } + return 1; +} + /** * Create a new key_def from a Lua table. * @@ -262,5 +382,18 @@ luaopen_key_def(struct lua_State *L) }; luaL_register_module(L, "key_def", meta); + lua_newtable(L); /* key_def.internal */ + lua_pushcfunction(L, lbox_key_def_extract_key); + lua_setfield(L, -2, "extract_key"); + lua_pushcfunction(L, lbox_key_def_compare); + lua_setfield(L, -2, "compare"); + lua_pushcfunction(L, lbox_key_def_compare_with_key); + lua_setfield(L, -2, "compare_with_key"); + lua_pushcfunction(L, lbox_key_def_merge); + lua_setfield(L, -2, "merge"); + lua_pushcfunction(L, lbox_key_def_to_table); + lua_setfield(L, -2, "to_table"); + lua_setfield(L, -2, "internal"); + return 1; } diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h index 61894d452..7dc7158e8 100644 --- a/src/box/lua/key_def.h +++ b/src/box/lua/key_def.h @@ -36,6 +36,13 @@ extern "C" { #endif /* defined(__cplusplus) */ struct lua_State; +struct key_part; + +/** + * Push a new table representing a key_part to a Lua stack. + */ +void +lbox_push_key_part(struct lua_State *L, const struct key_part *part); /** * Extract a key_def object from a Lua stack. diff --git a/src/box/lua/key_def.lua b/src/box/lua/key_def.lua new file mode 100644 index 000000000..1bf76e528 --- /dev/null +++ b/src/box/lua/key_def.lua @@ -0,0 +1,19 @@ +local ffi = require('ffi') +local key_def = require('key_def') +local key_def_t = ffi.typeof('struct key_def') + +local methods = { + ['extract_key'] = key_def.internal.extract_key, + ['compare'] = key_def.internal.compare, + ['compare_with_key'] = key_def.internal.compare_with_key, + ['merge'] = key_def.internal.merge, + ['to_table'] = key_def.internal.to_table, + ['__serialize'] = key_def.internal.to_table, +} + +ffi.metatype(key_def_t, { + __index = function(self, key) + return methods[key] + end, + __tostring = function(key_def) return "<struct key_def &>" end, +}) diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc index 9dfc97b6a..dbcdc9300 100644 --- a/src/box/lua/space.cc +++ b/src/box/lua/space.cc @@ -30,6 +30,7 @@ */ #include "box/lua/space.h" #include "box/lua/tuple.h" +#include "box/lua/key_def.h" #include "box/sql/sqlLimit.h" #include "lua/utils.h" #include "lua/trigger.h" @@ -286,32 +287,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i) for (uint32_t j = 0; j < index_def->key_def->part_count; j++) { lua_pushnumber(L, j + 1); - lua_newtable(L); - const struct key_part *part = - &index_def->key_def->parts[j]; - - lua_pushstring(L, field_type_strs[part->type]); - lua_setfield(L, -2, "type"); - - lua_pushnumber(L, part->fieldno + TUPLE_INDEX_BASE); - lua_setfield(L, -2, "fieldno"); - - if (part->path != NULL) { - lua_pushlstring(L, part->path, part->path_len); - lua_setfield(L, -2, "path"); - } - - lua_pushboolean(L, key_part_is_nullable(part)); - lua_setfield(L, -2, "is_nullable"); - - if (part->coll_id != COLL_NONE) { - struct coll_id *coll_id = - coll_by_id(part->coll_id); - assert(coll_id != NULL); - lua_pushstring(L, coll_id->name); - lua_setfield(L, -2, "collation"); - } - + lbox_push_key_part(L, &index_def->key_def->parts[j]); lua_settable(L, -3); /* index[k].parts[j] */ } diff --git a/test/box-tap/key_def.test.lua b/test/box-tap/key_def.test.lua index fd5d60b18..c22ef62a8 100755 --- a/test/box-tap/key_def.test.lua +++ b/test/box-tap/key_def.test.lua @@ -123,7 +123,7 @@ local cases = { local test = tap.test('key_def') -test:plan(#cases - 1) +test:plan(#cases - 1 + 16) for _, case in ipairs(cases) do if type(case) == 'function' then case() @@ -145,4 +145,45 @@ for _, case in ipairs(cases) do end end +-- +-- gh-4025: Introduce built-in function to get index key +-- from tuple. +-- gh-3398: Access to index compare function from lua. +-- +tuple_a = box.tuple.new({1, 1, 22}) +tuple_b = box.tuple.new({2, 1, 11}) +tuple_c = box.tuple.new({3, 1, 22}) +pk_parts = {{type = 'unsigned', fieldno = 1}} +sk_parts = {{type = 'number', fieldno=2}, {type='number', fieldno=3}} +pk_def = key_def.new(pk_parts) +test:is_deeply(pk_def:extract_key(tuple_a):totable(), {1}, "pk extract") +sk_def = key_def.new(sk_parts) +test:is_deeply(sk_def:extract_key(tuple_a):totable(), {1, 22}, "sk extract") + +test:is(pk_def:compare(tuple_b, tuple_a), 1, "pk compare 1") +test:is(pk_def:compare(tuple_b, tuple_c), -1, "pk compare 2") +test:is(sk_def:compare(tuple_b, tuple_a), -1, "sk compare 1") +test:is(sk_def:compare(tuple_a, tuple_c), 0, "sk compare 1") + +pk_sk_def = pk_def:merge(sk_def) +test:ok(pk_sk_def, "pksk merge") +test:is_deeply(pk_sk_def:extract_key(tuple_a):totable(), {1, 1, 22}, + "pksk compare 1") +test:is(pk_sk_def:compare(tuple_a, tuple_b), -1, "pksk compare 2") +sk_pk_def = sk_def:merge(pk_def) +test:ok(pk_sk_def, "skpk merge") +test:is_deeply(sk_pk_def:extract_key(tuple_a):totable(), {1, 22, 1}, + "skpk compare 1") +test:is(sk_pk_def:compare(tuple_a, tuple_b), 1, "skpk compare 1") + +key = sk_def:extract_key(tuple_a) +test:is(sk_def:compare_with_key(tuple_a, key), 0, + "sk compare_with_key - extracted tuple key") +test:is(sk_def:compare_with_key(tuple_a, {1, 22}), 0, + "sk compare_with_key - table key") +test:is_deeply(sk_def:to_table(), + {{type='number', fieldno=2, is_nullable=false}, + {type='number', fieldno=3, is_nullable=false}}, "sk to_table") +test:is(tostring(sk_def) == "<struct key_def &>", true, "tostring(sk_def)") + os.exit(test:check() and 0 or 1) -- 2.21.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object 2019-03-26 13:37 ` Kirill Shcherbatov @ 2019-03-26 22:54 ` Alexander Turenko 2019-04-23 15:06 ` Konstantin Osipov 0 siblings, 1 reply; 7+ messages in thread From: Alexander Turenko @ 2019-03-26 22:54 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: tarantool-patches I would prefer if you will answer to review comments explicitly. Re using lua_tolstring(): I understood that lua_tolstring() only guarantees that a string is valid until it will be popped from a stack (despite that fact that we know it has another copy on the stack) and so a region usage looks good. Just for the record: We discussed voicely that make this patch depending on the luaT_tuple_new() implementation from the merger patchset is not worth to do (just saves two lines of code here). Changes I made (I mostly add them into a separate 'FIXUP' commit to allow you to look into them if you want; pushed to your branch back): * Squashed the commits, updated the commit message. * Renamed :to_table() to totable(). * Use memcpy(tmp, path, path_len + 1) instead of an explicit '\0' assignment (lua_tolstring() allows it). * Use 'struct lua_State' instead of just 'lua_State' to better match with our code style. * Rewrote a test: split it by cases, do some renaming, fixed mine parts = {...} to parts = {{...}}, added more cases. * Found an assertion fault with a new test case: tarantool: /home/alex/p/tarantool-meta/r/tarantool/src/box/tuple_extract_key.cc:140: char* tuple_extract_key_slowpath(const tuple*, key_def*, uint32_t*) [with bool contains_sequential_parts = false; bool has_optional_parts = false; bool has_json_paths = false; uint32_t = unsigned int]: Assertion `field != NULL' failed. Aborted See the first commented test case. I think that we should allow a fieldno in key_def that is above then a tuple length when the field is nullable and raise an error otherwise (when it is not nullable). (I guess that we can do it at least partially by setting has_optional_parts). However when I tried with non-nullable part with {fieldno = 2} :extract_key({'foo'}) gives me {'foo', 80} -- it seems to go over its memory. But it asserts like the first one if I added {fieldno = 3}. All that cases are commented now. Can you please elaborate how to archieve the behaviour I proposed above and whether it is good way to go, Kirill? Also I have an idea: maybe add ability to use tables as tuples? We have to receive tuples in this way from net.box. WBR, Alexander Turenko. On Tue, Mar 26, 2019 at 04:37:54PM +0300, Kirill Shcherbatov wrote: > Hi! Thank you for review. > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object 2019-03-26 22:54 ` Alexander Turenko @ 2019-04-23 15:06 ` Konstantin Osipov 2019-04-23 15:09 ` Kirill Shcherbatov 0 siblings, 1 reply; 7+ messages in thread From: Konstantin Osipov @ 2019-04-23 15:06 UTC (permalink / raw) To: tarantool-patches; +Cc: Kirill Shcherbatov * Alexander Turenko <alexander.turenko@tarantool.org> [19/03/27 09:35]: Could you please send the final patch for review? > I would prefer if you will answer to review comments explicitly. > > Re using lua_tolstring(): I understood that lua_tolstring() only > guarantees that a string is valid until it will be popped from a stack > (despite that fact that we know it has another copy on the stack) and so > a region usage looks good. > > Just for the record: We discussed voicely that make this patch depending > on the luaT_tuple_new() implementation from the merger patchset is not > worth to do (just saves two lines of code here). > > Changes I made (I mostly add them into a separate 'FIXUP' commit to > allow you to look into them if you want; pushed to your branch back): > > * Squashed the commits, updated the commit message. > * Renamed :to_table() to totable(). > * Use memcpy(tmp, path, path_len + 1) instead of an explicit '\0' > assignment (lua_tolstring() allows it). > * Use 'struct lua_State' instead of just 'lua_State' to better match > with our code style. > * Rewrote a test: split it by cases, do some renaming, fixed mine > parts = {...} to parts = {{...}}, added more cases. > * Found an assertion fault with a new test case: > > tarantool: /home/alex/p/tarantool-meta/r/tarantool/src/box/tuple_extract_key.cc:140: char* tuple_extract_key_slowpath(const tuple*, key_def*, uint32_t*) [with bool contains_sequential_parts = false; bool has_optional_parts = false; bool has_json_paths = false; uint32_t = unsigned int]: Assertion `field != NULL' failed. > Aborted > > See the first commented test case. I think that we should allow a > fieldno in key_def that is above then a tuple length when the field is > nullable and raise an error otherwise (when it is not nullable). (I > guess that we can do it at least partially by setting > has_optional_parts). > > However when I tried with non-nullable part with {fieldno = 2} > :extract_key({'foo'}) gives me {'foo', 80} -- it seems to go over its > memory. But it asserts like the first one if I added {fieldno = 3}. > > All that cases are commented now. Can you please elaborate how to > archieve the behaviour I proposed above and whether it is good way to > go, Kirill? > > Also I have an idea: maybe add ability to use tables as tuples? We have > to receive tuples in this way from net.box. > > WBR, Alexander Turenko. > > On Tue, Mar 26, 2019 at 04:37:54PM +0300, Kirill Shcherbatov wrote: > > Hi! Thank you for review. > > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods > > https://github.com/tarantool/tarantool/tree/kshch/gh-4025-lua-key-kef-methods-110 -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object 2019-04-23 15:06 ` Konstantin Osipov @ 2019-04-23 15:09 ` Kirill Shcherbatov 0 siblings, 0 replies; 7+ messages in thread From: Kirill Shcherbatov @ 2019-04-23 15:09 UTC (permalink / raw) To: tarantool-patches, Konstantin Osipov > Could you please send the final patch for review? Hi! I've already sent it yesterday https://www.freelists.org/post/tarantool-patches/PATCH-v3-11-lua-add-key-def-lua-module "[tarantool-patches] [PATCH v3 1/1] lua: add key_def lua module" 22.04.2019, 15:28 I may send it again, if you wish. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object 2019-03-25 12:27 [tarantool-patches] [PATCH v1 1/1] lua: export key_def methods for LUA key_def object Kirill Shcherbatov 2019-03-26 1:46 ` [tarantool-patches] " Alexander Turenko @ 2019-04-23 15:05 ` Konstantin Osipov 1 sibling, 0 replies; 7+ messages in thread From: Konstantin Osipov @ 2019-04-23 15:05 UTC (permalink / raw) To: tarantool-patches; +Cc: alexander.turenko, Kirill Shcherbatov * Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/03/25 15:29]: > Functions extract_key, compare, compare_with_key, merge defined > for key_def introduced for LUA key_def object. > > Close #4025 > > @TarantoolBot document > Title: Built-in methods for key_def object > Now it is possible to work with index base object - key > definition in LUA. Available methods: > key = key_def:extract_key(tuple) > key_def:compare(tuple_a, tuple_b) > key_def:compare_with_key(tuple, key), where key is tuple or table > new_key_def = key_def:merge(second_key_def) The API mostly looks good to me. I hesitated a bit about extract_key() building a tuple, not returning a raw msgpack pointer and part count, like C api, but then I erred on the side of safety. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-23 15:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-25 12:27 [tarantool-patches] [PATCH v1 1/1] lua: export key_def methods for LUA key_def object Kirill Shcherbatov 2019-03-26 1:46 ` [tarantool-patches] " Alexander Turenko 2019-03-26 13:37 ` Kirill Shcherbatov 2019-03-26 22:54 ` Alexander Turenko 2019-04-23 15:06 ` Konstantin Osipov 2019-04-23 15:09 ` Kirill Shcherbatov 2019-04-23 15:05 ` Konstantin Osipov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox