Re: [PATCH v3 1/1] lua: add key_def lua module

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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 <COPYRIGHT HOLDER> ``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
> + * <COPYRIGHT HOLDER> 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;
> +}