From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 530662A0FD for ; Mon, 25 Mar 2019 21:46:33 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id vYvEXgylpPfO for ; Mon, 25 Mar 2019 21:46:33 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 82DDB2A0F6 for ; Mon, 25 Mar 2019 21:46:32 -0400 (EDT) Date: Tue, 26 Mar 2019 04:46:25 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object Message-ID: <20190326014624.jzrpzwghnrtfx3mr@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org 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 > #include > +#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 "" 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 >