[tarantool-patches] Re: [PATCH v1 1/1] lua: export key_def methods for LUA key_def object

[Alexander Turenko]

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 <lua.h>
>  #include <lauxlib.h>
> +#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 "<struct key_def&>" 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
>