From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 10 Jan 2019 16:07:56 +0300 From: Vladimir Davydov Subject: Re: [PATCH v2 4/6] lua: add luaT_new_key_def() Message-ID: <20190110130756.iwtn6knjg5r4tizo@esperanza> References: <1e5e2d29d4b6789417211dd94cccd1f12fcce696.1547064388.git.alexander.turenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1e5e2d29d4b6789417211dd94cccd1f12fcce696.1547064388.git.alexander.turenko@tarantool.org> To: Alexander Turenko Cc: tarantool-patches@freelists.org List-ID: On Wed, Jan 09, 2019 at 11:20:12PM +0300, Alexander Turenko wrote: > The function is needed to create the new struct key_def from C code > using a Lua table in the format compatible with > box.space[...].index[...].parts and > net_box_conn.space[...].index[...].parts. > > Needed for #3276. > --- > extra/exports | 1 + > src/CMakeLists.txt | 1 + > src/box/CMakeLists.txt | 1 + > src/box/lua/key_def.c | 217 +++++++++++++++++++++++++++++++ > src/box/lua/key_def.h | 61 +++++++++ > test/app-tap/module_api.c | 13 ++ > test/app-tap/module_api.test.lua | 89 ++++++++++++- > 7 files changed, 381 insertions(+), 2 deletions(-) > create mode 100644 src/box/lua/key_def.c > create mode 100644 src/box/lua/key_def.h > > diff --git a/extra/exports b/extra/exports > index af6863963..497719ed8 100644 > --- a/extra/exports > +++ b/extra/exports > @@ -210,6 +210,7 @@ clock_realtime64 > clock_monotonic64 > clock_process64 > clock_thread64 > +luaT_new_key_def > > # Lua / LuaJIT > > diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt > index 04de5ad04..494c8d391 100644 > --- a/src/CMakeLists.txt > +++ b/src/CMakeLists.txt > @@ -202,6 +202,7 @@ set(api_headers > ${CMAKE_SOURCE_DIR}/src/lua/error.h > ${CMAKE_SOURCE_DIR}/src/box/txn.h > ${CMAKE_SOURCE_DIR}/src/box/key_def.h > + ${CMAKE_SOURCE_DIR}/src/box/lua/key_def.h > ${CMAKE_SOURCE_DIR}/src/box/field_def.h > ${CMAKE_SOURCE_DIR}/src/box/tuple.h > ${CMAKE_SOURCE_DIR}/src/box/tuple_format.h > diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt > index 5521e489e..0db093768 100644 > --- a/src/box/CMakeLists.txt > +++ b/src/box/CMakeLists.txt > @@ -139,6 +139,7 @@ add_library(box STATIC > lua/net_box.c > lua/xlog.c > lua/sql.c > + lua/key_def.c > ${bin_sources}) > > target_link_libraries(box box_error tuple stat xrow xlog vclock crc32 scramble > diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c > new file mode 100644 > index 000000000..60247f427 > --- /dev/null > +++ b/src/box/lua/key_def.c > @@ -0,0 +1,217 @@ > +/* > + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include "box/lua/key_def.h" > + > +#include > +#include > +#include "diag.h" > +#include "box/key_def.h" > +#include "box/box.h" > +#include "box/coll_id_cache.h" > +#include "lua/utils.h" > + > +struct key_def * > +luaT_new_key_def(struct lua_State *L, int idx) If you agree with luaT_tuple_new, then rename this function to luaT_key_def_new pls. > +{ > + if (lua_istable(L, idx) != 1) { > + luaL_error(L, "Bad params, use: luaT_new_key_def({" > + "{fieldno = fieldno, type = type" > + "[, is_nullable = is_nullable" > + "[, collation_id = collation_id" Hm, what's collation_id for? > + "[, collation = collation]]]}, ...}"); This looks like you can't specify collation without is_nullable. Should be luaT_new_key_def({{fieldno = FIELDNO, type = TYPE[, is_nullable = true | false][, collation = COLLATION]}}) > + unreachable(); > + return NULL; > + } > + uint32_t key_parts_count = 0; > + uint32_t capacity = 8; > + > + const ssize_t parts_size = sizeof(struct key_part_def) * capacity; Can't we figure out the table length right away instead of reallocaing key_part_def array? > + struct key_part_def *parts = NULL; > + parts = (struct key_part_def *) malloc(parts_size); > + if (parts == NULL) { > + diag_set(OutOfMemory, parts_size, "malloc", "parts"); > + luaT_error(L); > + unreachable(); > + return NULL; > + } > + > + while (true) { Would be nice to factor out part creation to a separate function. > + lua_pushinteger(L, key_parts_count + 1); We would call this variable key_part_count (without 's') or even just part_count, as you called the array of key parts simply 'parts'. > + lua_gettable(L, idx); > + if (lua_isnil(L, -1)) > + break; > + > + /* Extend parts if necessary. */ > + if (key_parts_count == capacity) { > + capacity *= 2; > + struct key_part_def *old_parts = parts; > + const ssize_t parts_size = > + sizeof(struct key_part_def) * capacity; > + parts = (struct key_part_def *) realloc(parts, > + parts_size); > + if (parts == NULL) { > + free(old_parts); > + diag_set(OutOfMemory, parts_size / 2, "malloc", > + "parts"); > + luaT_error(L); > + unreachable(); > + return NULL; > + } > + } > + > + /* Set parts[key_parts_count].fieldno. */ > + lua_pushstring(L, "fieldno"); > + lua_gettable(L, -2); > + if (lua_isnil(L, -1)) { > + free(parts); > + luaL_error(L, "fieldno must not be nil"); > + unreachable(); > + return NULL; > + } > + /* > + * Transform one-based Lua fieldno to zero-based > + * fieldno to use in key_def_new(). > + */ > + parts[key_parts_count].fieldno = lua_tointeger(L, -1) - 1; Use TUPLE_INDEX_BASE instead of 1 pls. > + lua_pop(L, 1); > + > + /* Set parts[key_parts_count].type. */ > + lua_pushstring(L, "type"); > + lua_gettable(L, -2); > + if (lua_isnil(L, -1)) { > + free(parts); > + luaL_error(L, "type must not be nil"); > + unreachable(); > + return NULL; > + } > + size_t type_len; > + const char *type_name = lua_tolstring(L, -1, &type_len); > + lua_pop(L, 1); > + parts[key_parts_count].type = field_type_by_name(type_name, > + type_len); > + if (parts[key_parts_count].type == field_type_MAX) { > + free(parts); > + luaL_error(L, "Unknown field type: %s", type_name); > + unreachable(); > + return NULL; > + } > + > + /* > + * Set parts[key_parts_count].is_nullable and > + * parts[key_parts_count].nullable_action. > + */ > + lua_pushstring(L, "is_nullable"); > + lua_gettable(L, -2); > + if (lua_isnil(L, -1)) { > + parts[key_parts_count].is_nullable = false; > + parts[key_parts_count].nullable_action = > + ON_CONFLICT_ACTION_DEFAULT; > + } else { > + parts[key_parts_count].is_nullable = > + lua_toboolean(L, -1); > + parts[key_parts_count].nullable_action = > + ON_CONFLICT_ACTION_NONE; > + } > + lua_pop(L, 1); > + > + /* Set parts[key_parts_count].coll_id using collation_id. */ > + lua_pushstring(L, "collation_id"); > + lua_gettable(L, -2); > + if (lua_isnil(L, -1)) > + parts[key_parts_count].coll_id = COLL_NONE; > + else > + parts[key_parts_count].coll_id = lua_tointeger(L, -1); > + lua_pop(L, 1); > + > + /* Set parts[key_parts_count].coll_id using collation. */ > + lua_pushstring(L, "collation"); > + lua_gettable(L, -2); > + /* Check whether box.cfg{} was called. */ Collations should be usable even without box.cfg IIRC. Well, not all of them I think, but still you don't need to check box.cfg() here AFAIU. > + if ((parts[key_parts_count].coll_id != COLL_NONE || > + !lua_isnil(L, -1)) && !box_is_configured()) { > + free(parts); > + luaL_error(L, "Cannot use collations: " > + "please call box.cfg{}"); > + unreachable(); > + return NULL; > + } > + if (!lua_isnil(L, -1)) { > + if (parts[key_parts_count].coll_id != COLL_NONE) { > + free(parts); > + luaL_error(L, "Conflicting options: " > + "collation_id and collation"); > + unreachable(); > + return NULL; > + } > + size_t coll_name_len; > + const char *coll_name = lua_tolstring(L, -1, > + &coll_name_len); > + struct coll_id *coll_id = coll_by_name(coll_name, > + coll_name_len); Ouch, this doesn't seem to belong here. Ideally, it should be done by key_def_new(). Can we rework key_part_def so that it stores collation string instead of collation id? > + if (coll_id == NULL) { > + free(parts); > + luaL_error(L, "Unknown collation: \"%s\"", > + coll_name); > + unreachable(); > + return NULL; > + } > + parts[key_parts_count].coll_id = coll_id->id; > + } > + lua_pop(L, 1); > + > + /* Check coll_id. */ > + struct coll_id *coll_id = > + coll_by_id(parts[key_parts_count].coll_id); > + if (parts[key_parts_count].coll_id != COLL_NONE && > + coll_id == NULL) { > + uint32_t collation_id = parts[key_parts_count].coll_id; > + free(parts); > + luaL_error(L, "Unknown collation_id: %d", collation_id); > + unreachable(); > + return NULL; > + } > + > + /* Set parts[key_parts_count].sort_order. */ > + parts[key_parts_count].sort_order = SORT_ORDER_ASC; > + > + ++key_parts_count; > + } > + > + struct key_def *key_def = key_def_new(parts, key_parts_count); > + free(parts); > + if (key_def == NULL) { > + luaL_error(L, "Cannot create key_def"); > + unreachable(); > + return NULL; > + } > + return key_def; > +} > diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h > new file mode 100644 > index 000000000..55292fb7e > --- /dev/null > +++ b/src/box/lua/key_def.h > @@ -0,0 +1,61 @@ > +#ifndef TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED > +#define TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED > +/* > + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#if defined(__cplusplus) > +extern "C" { > +#endif /* defined(__cplusplus) */ > + > +struct key_def; > +struct lua_State; > + > +/** \cond public */ > + > +/** > + * Create the new key_def from a Lua table. a key_def > + * > + * Expected a table of key parts on the Lua stack. The format is > + * the same as box.space.<...>.index.<...>.parts or corresponding > + * net.box's one. > + * > + * Returns the new key_def. > + */ > +struct key_def * > +luaT_new_key_def(struct lua_State *L, int idx); > + > +/** \endcond public */ > + > +#if defined(__cplusplus) > +} /* extern "C" */ > +#endif /* defined(__cplusplus) */ > + > +#endif /* TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED */ > diff --git a/test/app-tap/module_api.c b/test/app-tap/module_api.c > index b81a98056..34ab54bc0 100644 > --- a/test/app-tap/module_api.c > +++ b/test/app-tap/module_api.c > @@ -449,6 +449,18 @@ test_iscallable(lua_State *L) > return 1; > } > > +static int > +test_luaT_new_key_def(lua_State *L) > +{ > + /* > + * Ignore the return value. Here we test whether the > + * function raises an error. > + */ > + luaT_new_key_def(L, 1); It would be nice to test that it actually creates a valid key_def. Testing error conditions is less important. > + lua_pop(L, 1); > + return 0; > +} > + > LUA_API int > luaopen_module_api(lua_State *L) > { > @@ -477,6 +489,7 @@ luaopen_module_api(lua_State *L) > {"test_state", test_state}, > {"test_tostring", test_tostring}, > {"iscallable", test_iscallable}, > + {"luaT_new_key_def", test_luaT_new_key_def}, > {NULL, NULL} > }; > luaL_register(L, "module_api", lib);