[tarantool-patches] Re: [PATCH v3 4/4] box: specify indexes in user-friendly form

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! See 3 comments below.

> diff --git a/src/box/lua/index.c b/src/box/lua/index.c
> index ef89c39..b2b8ca2 100644
> --- a/src/box/lua/index.c
> +++ b/src/box/lua/index.c
> @@ -328,6 +331,76 @@ lbox_index_compact(lua_State *L)
>   	return 0;
>   }
>   
> +static int
> +lbox_index_resolve_path(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 3 ||
> +	    !lua_isnumber(L, 1) || !lua_isnumber(L, 2) || !lua_isstring(L, 3)) {
> +		return luaL_error(L, "Usage box.internal."
> +				     "path_resolve(part_id, space_id, path)");
> +	}
> +	uint32_t part_id = lua_tonumber(L, 1);
> +	uint32_t space_id = lua_tonumber(L, 2);
> +	const char *path = lua_tostring(L, 3);
> +	size_t path_len = strlen(path);
> +	struct space *space = space_cache_find(space_id);
> +	if (space == NULL)
> +		return luaT_error(L);
> +	struct json_path_parser parser;
> +	struct json_path_node node;
> +	json_path_parser_create(&parser, path, path_len);
> +	int rc = json_path_next(&parser, &node);
> +	if (rc != 0) {
> +		const char *err_msg =
> +			tt_sprintf("Illegal parameters, options.parts[%d]: "
> +				   "error in path on position %d", part_id, rc);
> +		diag_set(IllegalParams, err_msg);

1. It is box sub-module so use ClientError please.

> +		return luaT_error(L);
> +	}
> +	assert(space->format != NULL && space->format->dict != NULL);
> +	uint32_t fieldno = node.num - TUPLE_INDEX_BASE;;

2. How do you use node.num before checking that it is num? And why
referencing a field out of format is forbidden? I am free to define
indexes on non-formatted fields.

> +	if (node.type == JSON_PATH_NUM &&
> +	    fieldno >= space->format->field_count) {
> +		const char *err_msg =
> +			tt_sprintf("Illegal parameters, options.parts[%d]: "
> +				   "field '%d' referenced in path is greater "
> +				   "than format field count %d", part_id,
> +				   fieldno + TUPLE_INDEX_BASE,
> +				   space->format->field_count);
> +		diag_set(IllegalParams, err_msg);
> +		return luaT_error(L);
> +	} else if (node.type == JSON_PATH_STR &&
> +		   tuple_fieldno_by_name(space->format->dict, node.str, node.len,
> +					 field_name_hash(node.str, node.len),
> +					 &fieldno) != 0) {
> +		const char *err_msg =
> +			tt_sprintf("Illegal parameters, options.parts[%d]: "
> +				   "field was not found by name '%.*s'",
> +				   part_id, node.len, node.str);
> +		diag_set(IllegalParams, err_msg);
> +		return luaT_error(L);
> +	}
> +	fieldno += TUPLE_INDEX_BASE;
> +
> +	char *path_resolved = region_alloc(&fiber()->gc, path_len + 1);
> +	if (path_resolved == NULL) {
> +		diag_set(OutOfMemory, path_len + 1, "region_alloc",
> +			"path_resolved");
> +		return luaT_error(L);
> +	}
> +
> +	path = path_resolved;
> +	path_resolved +=
> +		sprintf(path_resolved, "[%d]", fieldno);

3. Fits in one line.

> +	memcpy(path_resolved, parser.src + parser.offset,
> +		parser.src_len - parser.offset);
> +	path_resolved[parser.src_len - parser.offset] = '\0';
> +
> +	lua_pushnumber(L, fieldno);
> +	lua_pushstring(L, path);
> +	return 2;
> +}
> +
>   /* }}} */
>   
>   void