From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <kshcherbatov@tarantool.org>
Subject: Re: [tarantool-patches] Re: [PATCH v5 12/12] box: specify indexes in
 user-friendly form
References: <cover.1540795996.git.kshcherbatov@tarantool.org>
 <7c70ea3b40c3eded10145be1c88b10242a18da24.1540795997.git.kshcherbatov@tarantool.org>
 <20181101141803.GD30032@chai>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Message-ID: <0872f05d-860b-d098-fe7d-f830c892d9ed@tarantool.org>
Date: Tue, 6 Nov 2018 15:15:15 +0300
MIME-Version: 1.0
In-Reply-To: <20181101141803.GD30032@chai>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
To: tarantool-patches@freelists.org, Kostya Osipov <kostja@tarantool.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>
List-ID: <tarantool-patches.dev.tarantool.org>

> No comment for the function. What does this function do and what
> is it used for?
/**
 * Resolve field index by absolute JSON path first component and
 * return relative JSON path.
 */
> 
>> +	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)");
> 
> Why is it called path_resolve in one place and resolve_path in
> another? 
Ok, lbox_index_path_resolve
> 
>> -                end
>> -            end
>> -            if type(part.field) == 'string' then
>> +            local idx, path = box.internal.path_resolve(i, space_id, part.field)
>> +            if part.path ~= nil and part.path ~= path then
>>                  box.error(box.error.ILLEGAL_PARAMS,
>> -                          "options.parts[" .. i .. "]: field was not found by name '" .. part.field .. "'")
>> +                          "options.parts[" .. i .. "]: field path '"..
>> +                          part.path.." doesn't math path resolved by name '" ..
> 
> Doesn't match the path
Ok.

> 
> Please check with the docs team all the new messages this patch is
> adding to the server.
I'll ask Lena when the patch is ready. 

> I don't see how this cross-check help. I can change space format
> later on. Looks like we need to push the validation to alter.cc to
> ensure any kind of consistency.
> 
> Generally, as a rule, we try to avoid referencing anything by
> name, and prefer referencing by id, even though the user can use
> the name in box.* api. This spares us from the responsibility to 
> cross-check all the referencing objects whenever a referenced
> object changes.
> 
> What is the strategy for json paths here? Could you describe it in
> a comment? 
There are two ways to define JSON path:
1) manually specify fieldno and relative path -- this is cannonical approach introduced in
previous commits.
idx = s:create_index('test1', {parts = {{3, 'str', path = 'FIO.fname'}}})
2) use a fastpath introduced with this patch:
idx = s:create_index('test1', {parts = {{"user.FIO.fname", 'str'}}})

The second way is just user-friendly interface, string "user.FIO.fname" is not persisted in
Tarantool and indirectly converted to the (1) form in this patch.