Re: [PATCH v3 3/3] Lua: implement json path access to tuple fields

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

On Tue, Apr 10, 2018 at 11:31:46AM +0300, Vladislav Shpilevoy wrote:
> From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
> 
> New tuple_field_raw_by_path and tuple_field_by_path APIs.

Would be nice to say a few words here about the new API.

> 
> Resolves #1285
> ---
>  src/box/CMakeLists.txt     |   2 +-
>  src/box/lua/tuple.c        |  63 +++++++++----
>  src/box/lua/tuple.lua      |  49 +++-------
>  src/box/tuple.h            |  21 +++++
>  src/box/tuple_format.c     | 164 ++++++++++++++++++++++++++++++++
>  src/box/tuple_format.h     |  19 ++++
>  test/engine/tuple.result   | 229 +++++++++++++++++++++++++++++++++++++++++++++
>  test/engine/tuple.test.lua |  67 +++++++++++++
>  8 files changed, 558 insertions(+), 56 deletions(-)
> 
> diff --git a/src/box/CMakeLists.txt b/src/box/CMakeLists.txt
> index ef7225d10..ae156a2f5 100644
> --- a/src/box/CMakeLists.txt
> +++ b/src/box/CMakeLists.txt
> @@ -47,7 +47,7 @@ add_library(tuple STATIC
>      field_def.c
>      opt_def.c
>  )
> -target_link_libraries(tuple box_error core ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES} misc bit)
> +target_link_libraries(tuple json_path box_error core ${MSGPUCK_LIBRARIES} ${ICU_LIBRARIES} misc bit)
>  
>  add_library(xlog STATIC xlog.c)
>  target_link_libraries(xlog core box_error crc32 ${ZSTD_LIBRARIES})
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index a0b4e4a79..c10e9b253 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -41,6 +41,7 @@
>  #include "box/tuple_convert.h"
>  #include "box/errcode.h"
>  #include "box/memtx_tuple.h"
> +#include "json/path.h"
>  
>  /** {{{ box.tuple Lua library
>   *
> @@ -420,36 +421,58 @@ lbox_tuple_transform(struct lua_State *L)
>  }
>  
>  /**
> - * Find a tuple field using its name.
> + * Find a tuple field by JSON path. If a field was not found and a
> + * path contains JSON syntax errors, then an exception is raised.
>   * @param L Lua state.
> - * @param tuple 1-th argument on lua stack, tuple to get field
> + * @param tuple 1-th argument on a lua stack, tuple to get field
>   *        from.
> - * @param field_name 2-th argument on lua stack, field name to
> - *        get.
> + * @param path 2-th argument on lua stack. Can be field name,
> + *        JSON path to a field or a field number.
>   *
> - * @retval If a field was not found, return -1 and nil to lua else
> - *         return 0 and decoded field.
> + * @retval not nil Found field value.
> + * @retval     nil A field is NULL or does not exist.
>   */
>  static int
> -lbox_tuple_field_by_name(struct lua_State *L)
> +lbox_tuple_field_by_path(struct lua_State *L)
>  {
>  	struct tuple *tuple = luaT_istuple(L, 1);
>  	/* Is checked in Lua wrapper. */
>  	assert(tuple != NULL);
> -	assert(lua_isstring(L, 2));
> -	size_t name_len;
> -	const char *name = lua_tolstring(L, 2, &name_len);
> -	uint32_t name_hash = lua_hashstring(L, 2);
> -	const char *field =
> -		tuple_field_by_name(tuple, name, name_len, name_hash);
> -	if (field == NULL) {
> -		lua_pushinteger(L, -1);
> -		lua_pushnil(L);
> -		return 2;
> +	const char *field = NULL;
> +	if (lua_isnumber(L, 2)) {
> +		double dbl_index = lua_tonumber(L, 2);
> +		if (dbl_index != floor(dbl_index))
> +			goto usage_error;
> +		int index = (int) floor(dbl_index) - TUPLE_INDEX_BASE;
> +		if (index >= 0) {
> +			field = tuple_field(tuple, index);
> +			if (field == NULL) {
> +				lua_pushnil(L);
> +				return 1;
> +			}
> +		} else {
> +			lua_pushnil(L);
> +			return 1;
> +		}

Why did you move handling of integer index from Lua to C? AFAIU having
it in Lua should be faster. Besides, this new function is kinda
difficult to follow IMO as it does two things now. I'd prefer it to do
just one thing - looking up a tuple field by path.

Other than this, the patch looks good to me.

> +	} else if (lua_isstring(L, 2)) {
> +		size_t len;
> +		const char *path = lua_tolstring(L, 2, &len);
> +		if (len == 0)
> +			goto usage_error;
> +		if (tuple_field_by_path(tuple, path, (uint32_t) len,
> +					lua_hashstring(L, 2), &field) != 0) {
> +			return luaT_error(L);
> +		} else if (field == NULL) {
> +			lua_pushnil(L);
> +			return 1;
> +		}
> +	} else {
> +usage_error:
> +		return luaL_error(L, "Usage: tuple[<path> or number >= 1]");
>  	}
> -	lua_pushinteger(L, 0);
> +	assert(field != NULL);
>  	luamp_decode(L, luaL_msgpack_default, &field);
> -	return 2;
> +	return 1;
>  }