From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 30 Apr 2019 13:30:24 +0300 From: Vladimir Davydov Subject: Re: [PATCH v3 1/1] lua: add key_def lua module Message-ID: <20190430103024.jbzedrf3ak2plvjm@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org, alexander.turenko@tarantool.org List-ID: On Mon, Apr 22, 2019 at 03:28:55PM +0300, Kirill Shcherbatov wrote: > diff --git a/src/box/errcode.h b/src/box/errcode.h > index 3f8cb8e0e..72485d3c8 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -246,6 +246,7 @@ struct errcode_record { > /*191 */_(ER_SQL_PARSER_LIMIT, "%s %d exceeds the limit (%d)") \ > /*192 */_(ER_INDEX_DEF_UNSUPPORTED, "%s are prohibited in an index definition") \ > /*193 */_(ER_CK_DEF_UNSUPPORTED, "%s are prohibited in a CHECK constraint definition") \ > + /*194 */_(ER_TUPLE_KEY_PART_MISSED, "Supplied tuple field for part %u does not exists") \ We have ER_FIELD_MISSING, ER_NO_SUCH_FIELD_NO, ER_NO_SUCN_FIELD_NAME. Please reuse rather than introducing yet another error code. Also the error message should include a path to the missing field. > > /* > * !IMPORTANT! Please follow instructions at start of the file > diff --git a/src/box/lua/init.c b/src/box/lua/init.c > index 3ff4f3227..6b8be5096 100644 > --- a/src/box/lua/init.c > +++ b/src/box/lua/init.c > @@ -59,9 +59,11 @@ > #include "box/lua/console.h" > #include "box/lua/tuple.h" > #include "box/lua/execute.h" > +#include "box/lua/key_def.h" > > extern char session_lua[], > tuple_lua[], > + key_def_lua[], > schema_lua[], > load_cfg_lua[], > xlog_lua[], > @@ -80,6 +82,7 @@ static const char *lua_sources[] = { > "box/console", console_lua, > "box/load_cfg", load_cfg_lua, > "box/xlog", xlog_lua, > + "box/key_def", key_def_lua, > NULL > }; > > @@ -312,6 +315,8 @@ box_lua_init(struct lua_State *L) > lua_pop(L, 1); > tarantool_lua_console_init(L); > lua_pop(L, 1); > + luaopen_key_def(L); > + lua_pop(L, 1); > > /* Load Lua extension */ > for (const char **s = lua_sources; *s; s += 2) { > diff --git a/src/box/lua/key_def.c b/src/box/lua/key_def.c > new file mode 100644 > index 000000000..d5fe237c0 > --- /dev/null > +++ b/src/box/lua/key_def.c > @@ -0,0 +1,425 @@ > +/* > + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``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/coll_id_cache.h" > +#include "box/lua/key_def.h" > +#include "box/tuple.h" > +#include "diag.h" > +#include "fiber.h" > +#include "lua/utils.h" > +#include "misc.h" > +#include "tuple.h" > + > +static uint32_t key_def_type_id = 0; Here are the names of other variables storing Lua type ids: CTID_STRUCT_IBUF CTID_STRUCT_PORT_PTR CTID_STRUCT_ITERATOR_REF CTID_STRUCT_ERROR_REF Please rename this one to conform. > + > +void > +luaT_push_key_def(struct lua_State *L, const struct key_def *key_def) > +{ > + lua_createtable(L, key_def->part_count, 0); > + for (uint32_t i = 0; i < key_def->part_count; ++i) { > + const struct key_part *part = &key_def->parts[i]; > + 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"); > + } > + lua_rawseti(L, -2, i + 1); > + } > +} > + > +/** > + * Set key_part_def from a table on top of a Lua stack. > + * The region argument is used to allocate a JSON path when > + * required. > + * 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, > + struct region *region) > +{ I typed this by mistake: tarantool> key_def.new({fieldno = 1, type = 'unsigned'}) and got a valid key def: --- - [] ... Can we possibly raise an error in this case? > + *part = key_part_def_default; > + > + /* Set part->fieldno. */ > + lua_pushstring(L, "fieldno"); > + lua_gettable(L, -2); > + if (lua_isnil(L, -1)) { > + diag_set(IllegalParams, "fieldno must not be nil"); > + return -1; > + } > + /* > + * Transform one-based Lua fieldno to zero-based > + * fieldno to use in key_def_new(). > + */ > + part->fieldno = lua_tointeger(L, -1) - TUPLE_INDEX_BASE; > + lua_pop(L, 1); > + > + /* Set part->type. */ > + lua_pushstring(L, "type"); > + lua_gettable(L, -2); > + if (lua_isnil(L, -1)) { > + diag_set(IllegalParams, "type must not be nil"); > + return -1; > + } > + size_t type_len; > + const char *type_name = lua_tolstring(L, -1, &type_len); > + lua_pop(L, 1); > + part->type = field_type_by_name(type_name, type_len); > + switch (part->type) { > + case FIELD_TYPE_ANY: > + case FIELD_TYPE_ARRAY: > + case FIELD_TYPE_MAP: > + /* Tuple comparators don't support these types. */ > + diag_set(IllegalParams, "Unsupported field type: %s", > + type_name); > + return -1; > + case field_type_MAX: > + diag_set(IllegalParams, "Unknown field type: %s", type_name); > + return -1; > + default: > + /* Pass though. */ > + break; > + } > + > + /* Set part->is_nullable and part->nullable_action. */ > + lua_pushstring(L, "is_nullable"); > + lua_gettable(L, -2); > + if (!lua_isnil(L, -1) && lua_toboolean(L, -1) != 0) { > + part->is_nullable = true; > + part->nullable_action = ON_CONFLICT_ACTION_NONE; > + } > + lua_pop(L, 1); > + > + /* > + * Set part->coll_id using collation_id. > + * > + * The value will be checked in key_def_new(). > + */ > + lua_pushstring(L, "collation_id"); > + lua_gettable(L, -2); > + if (!lua_isnil(L, -1)) > + part->coll_id = lua_tointeger(L, -1); > + lua_pop(L, 1); > + > + /* Set part->coll_id using collation. */ > + lua_pushstring(L, "collation"); > + lua_gettable(L, -2); > + if (!lua_isnil(L, -1)) { > + /* Check for conflicting options. */ > + if (part->coll_id != COLL_NONE) { > + diag_set(IllegalParams, "Conflicting options: " > + "collation_id and collation"); > + return -1; > + } > + > + size_t coll_name_len; > + const char *coll_name = lua_tolstring(L, -1, &coll_name_len); > + struct coll_id *coll_id = coll_by_name(coll_name, > + coll_name_len); > + if (coll_id == NULL) { > + diag_set(IllegalParams, "Unknown collation: \"%s\"", > + coll_name); > + return -1; > + } > + part->coll_id = coll_id->id; > + } > + lua_pop(L, 1); > + > + /* Set part->path (JSON path). */ > + lua_pushstring(L, "path"); > + lua_gettable(L, -2); > + if (!lua_isnil(L, -1)) { > + size_t path_len; > + const char *path = lua_tolstring(L, -1, &path_len); > + if (json_path_validate(path, path_len, TUPLE_INDEX_BASE) != 0) { > + diag_set(IllegalParams, "invalid path"); > + return -1; > + } > + char *tmp = region_alloc(region, path_len + 1); > + if (tmp == NULL) { > + diag_set(OutOfMemory, path_len + 1, "region", "path"); > + return -1; > + } > + /* > + * lua_tolstring() guarantees that a string have > + * trailing '\0'. > + */ > + memcpy(tmp, path, path_len + 1); > + part->path = tmp; > + } else { > + part->path = NULL; > + } > + lua_pop(L, 1); > + return 0; > +} > + > +static struct tuple * > +luaT_key_def_check_tuple(struct lua_State *L, struct key_def *key_def, int idx) Please write a comment explaining briefly what this function does. Relevant to all the static functions below. > +{ > + struct tuple *tuple = luaT_istuple(L, idx); > + if (tuple == NULL) > + tuple = luaT_tuple_new(L, idx, box_tuple_format_default()); > + if (tuple == NULL || tuple_validate_parts(key_def, tuple) != 0) > + return NULL; > + tuple_ref(tuple); > + return tuple; > +} > + > +struct key_def * > +luaT_check_key_def(struct lua_State *L, int idx) > +{ > + if (lua_type(L, idx) != LUA_TCDATA) > + return NULL; > + > + uint32_t cdata_type; > + struct key_def **key_def_ptr = luaL_checkcdata(L, idx, &cdata_type); > + if (key_def_ptr == NULL || cdata_type != key_def_type_id) > + return NULL; > + return *key_def_ptr; > +} > + > +/** > + * Free a key_def from a Lua code. > + */ > +static int > +lbox_key_def_gc(struct lua_State *L) > +{ > + struct key_def *key_def = luaT_check_key_def(L, 1); > + if (key_def == NULL) > + return 0; Hmm, how's that even possible? > + box_key_def_delete(key_def); > + return 0; > +} > + > +static int > +lbox_key_def_extract_key(struct lua_State *L) > +{ > + struct key_def *key_def; > + if (lua_gettop(L) != 2 || (key_def = luaT_check_key_def(L, 1)) == NULL) > + return luaL_error(L, "Usage: key_def:extract_key(tuple)"); > + > + struct tuple *tuple; > + if ((tuple = luaT_key_def_check_tuple(L, key_def, 2)) == NULL) > + return luaT_error(L); > + > + uint32_t key_size; > + char *key = tuple_extract_key(tuple, key_def, &key_size); tuple_extract_key allocates a key on the region. You ought to clean it up once you're done with it. > + tuple_unref(tuple); > + if (key == NULL) > + return luaT_error(L); > + > + struct tuple *ret = > + box_tuple_new(box_tuple_format_default(), key, key + key_size); Please don't use box_ functions here - those are intended to be used only by external modules. Use tuple_new, tuple_format_runtime directly. It's especially confusing when you create a key def with key_def_new, but free it with box_key_def_delete. Anyway, do we really need to create a tuple here? May be returning a Lua table would be fine? > + if (ret == NULL) > + return luaT_error(L); > + luaT_pushtuple(L, ret); > + return 1; > +} > + > +static int > +lbox_key_def_compare(struct lua_State *L) > +{ > + struct key_def *key_def; > + if (lua_gettop(L) != 3 || > + (key_def = luaT_check_key_def(L, 1)) == NULL) { > + return luaL_error(L, "Usage: key_def:" > + "compare(tuple_a, tuple_b)"); > + } > + > + struct tuple *tuple_a, *tuple_b; > + if ((tuple_a = luaT_key_def_check_tuple(L, key_def, 2)) == NULL) > + return luaT_error(L); > + if ((tuple_b = luaT_key_def_check_tuple(L, key_def, 3)) == NULL) { > + tuple_unref(tuple_a); > + return luaT_error(L); > + } > + > + int rc = tuple_compare(tuple_a, tuple_b, key_def); > + tuple_unref(tuple_a); > + tuple_unref(tuple_b); > + lua_pushinteger(L, rc); > + return 1; > +} > + > +static int > +lbox_key_def_compare_with_key(struct lua_State *L) > +{ > + struct key_def *key_def; > + if (lua_gettop(L) != 3 || > + (key_def = luaT_check_key_def(L, 1)) == NULL) { > + return luaL_error(L, "Usage: key_def:" > + "compare_with_key(tuple, key)"); > + } > + > + struct tuple *tuple = luaT_key_def_check_tuple(L, key_def, 2); > + if (tuple == NULL) > + return luaT_error(L); > + > + size_t key_len; > + const char *key = lbox_encode_tuple_on_gc(L, 3, &key_len); Again, this function allocates a key on the region. Please clean it up. > + uint32_t part_count = mp_decode_array(&key); > + if (key_validate_parts(key_def, key, part_count, true) != 0) { > + tuple_unref(tuple); > + return luaT_error(L); > + } > + > + int rc = tuple_compare_with_key(tuple, key, part_count, key_def); > + tuple_unref(tuple); > + lua_pushinteger(L, rc); > + return 1; > +} > diff --git a/src/box/lua/key_def.h b/src/box/lua/key_def.h > new file mode 100644 > index 000000000..2cecfaae5 > --- /dev/null > +++ b/src/box/lua/key_def.h > @@ -0,0 +1,63 @@ > +#ifndef TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED > +#define TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED > +/* > + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY AUTHORS ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * AUTHORS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#if defined(__cplusplus) > +extern "C" { > +#endif /* defined(__cplusplus) */ > + > +struct lua_State; > +struct key_def; > + > +/** > + * Push a new table representing a key_def to a Lua stack. > + */ > +void > +luaT_push_key_def(struct lua_State *L, const struct key_def *key_def); > + > +/** > + * Extract a key_def object from a Lua stack. The comment is insufficient: what happens if the object isn't a key def? > + */ > +struct key_def * > +luaT_check_key_def(struct lua_State *L, int idx); > + > +/** > + * Register the module. > + */ > +int > +luaopen_key_def(struct lua_State *L); > + > +#if defined(__cplusplus) > +} /* extern "C" */ > +#endif /* defined(__cplusplus) */ > + > +#endif /* TARANTOOL_BOX_LUA_KEY_DEF_H_INCLUDED */ > diff --git a/src/box/tuple.h b/src/box/tuple.h > index eed8e1a34..a3ac45574 100644 > --- a/src/box/tuple.h > +++ b/src/box/tuple.h > @@ -670,6 +670,34 @@ tuple_field_by_part(struct tuple *tuple, struct key_part *part) > tuple_field_map(tuple), part); > } > > +/** > + * Check that tuple fields match with given key definition > + * key_def. > + * @param key_def Key definition. > + * @param tuple Tuple to validate. > + * > + * @retval 0 The tuple is valid. > + * @retval -1 The tuple is invalid. > + */ > +static inline int > +tuple_validate_parts(struct key_def *key_def, struct tuple *tuple) > +{ IMO tuple.h isn't a right place for this function: - First, it's relatively big and cold so I'd move it to a C file. - Second, it's about checking if a tuple conforms to a key_def while tuple.[hc] stores only basic struct tuple helpers and shouldn't depend on key_def (although sadly it does for now). The name is somewhat confusing too: we now have tuple_validate and tuple_validate_parts both of which validate tuple parts... What about - renaming it to tuple_validate_key_parts so as to emphasize that it's about key parts, not tuple parts; - declaring it in key_def.h while defining it in tuple_extract_key.cc, similarly to tuple_key_contains_null, as this function can be used for checking if we can extract a specific key from a given tuple ? > + for (uint32_t idx = 0; idx < key_def->part_count; idx++) { > + struct key_part *part = &key_def->parts[idx]; > + const char *field = tuple_field_by_part(tuple, part); > + if (field == NULL) { > + if (key_part_is_nullable(part)) > + continue; > + diag_set(ClientError, ER_TUPLE_KEY_PART_MISSED, idx); > + return -1; > + } > + if (key_part_validate(part->type, field, idx, > + key_part_is_nullable(part)) != 0) > + return -1; > + } > + return 0; > +}