Re: [PATCH v2 4/6] lua: add luaT_new_key_def()

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

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 <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/lua/key_def.h"
> +
> +#include <lua.h>
> +#include <lauxlib.h>
> +#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);