Re: [PATCH v8 5/5] box: specify indexes in user-friendly form

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

On Wed, Jan 16, 2019 at 04:44:43PM +0300, Kirill Shcherbatov wrote:
> Implemented a more convenient interface for creating an index
> by JSON path. Instead of specifying fieldno and relative path
> it comes possible to pass full JSON path to data.

it is now possible

> 
> Closes #1012
> 
> @TarantoolBot document
> Title: Indexes by JSON path
> Sometimes field data could have complex document structure.
> When this structure is consistent across whole space,
> you are able to create an index by JSON path.
> 
> Example:
> s = box.schema.space.create('sample')
> format = {{'id', 'unsigned'}, {'data', 'map'}}
> s:format(format)
> -- explicit JSON index creation
> age_idx = s:create_index('age', {{2, 'number', path = "age"}})
> -- user-friendly syntax for JSON index creation
> parts = {{'data.FIO["fname"]', 'str'}, {'data.FIO["sname"]', 'str'},
>          {'data.age', 'number'}}
> info_idx = s:create_index('info', {parts = parts}})
> s:insert({1, {FIO={fname="James", sname="Bond"}, age=35}})
> ---
>  src/box/lua/schema.lua    | 60 ++++++++++++++++++++++++++++++++++-----
>  test/engine/json.result   | 42 +++++++++++++++++++++++++++
>  test/engine/json.test.lua | 12 ++++++++
>  3 files changed, 107 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 8a804f0ba..a55ef5b22 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -575,6 +575,49 @@ local function update_index_parts_1_6_0(parts)
>      return result
>  end
>  

Please add comments to these functions.

> +local function format_field_index_by_name(format, name)
> +    for k,v in pairs(format) do
> +        if v.name == name then
> +            return k
> +        end
> +    end
> +    return nil
> +end
> +
> +local function format_field_resolve(format, name)
> +    local idx = nil
> +    local field_name = nil
> +    -- try resolve whole name

try to

> +    idx = format_field_index_by_name(format, name)
> +    if idx ~= nil then
> +        print("Case 1 "..idx)

What's this? Debugging prints? Looks like you didn't care to carefully
review your patches before sending them out... again.

> +        return idx, nil
> +    end
> +    -- try resolve [%d]
> +    field_name = string.match(name, "^%[(%d+)%]")
> +    idx = tonumber(field_name)
> +    if idx ~= nil then
> +        print("Case 2 "..idx..string.sub(name, string.len(field_name) + 3))
> +        return idx, string.sub(name, string.len(field_name) + 3)
> +    end
> +    -- try resolve ["%s"] or ['%s']
> +    field_name = string.match(name, "^%[\"(.*)\"%]") or
> +                 string.match(name, "^%[\'(.*)\'%]")

Here you use asterisk (*) while in [%d] case you use plus (+).
I understand there's actually no difference in this particular
case, but please be consistent.

> +    idx = format_field_index_by_name(format, field_name)
> +    if idx ~= nil then
> +        print("Case 3 "..idx..string.sub(name, string.len(field_name) + 5))
> +        return idx, string.sub(name, string.len(field_name) + 5)
> +    end
> +    -- try to resolve .*[ and .*.
> +    field_name = string.match(name, "^([^.%[]-)[.%[]")

And here you use minus (-) for some reason.
Wouldn't simple "^([^.[]+)" work?

> +    idx = format_field_index_by_name(format, field_name)
> +    if idx ~= nil then
> +        print("Case 4 "..idx..string.sub(name, string.len(field_name) + 1))
> +        return idx, string.sub(name, string.len(field_name) + 1)
> +    end
> +    return nil, nil
> +end
> +
>  local function update_index_parts(format, parts)
>      if type(parts) ~= "table" then
>          box.error(box.error.ILLEGAL_PARAMS,
> @@ -626,16 +669,19 @@ local function update_index_parts(format, parts)
>              box.error(box.error.ILLEGAL_PARAMS,
>                        "options.parts[" .. i .. "]: field (name or number) is expected")
>          elseif type(part.field) == 'string' then
> -            for k,v in pairs(format) do
> -                if v.name == part.field then
> -                    part.field = k
> -                    break
> -                end
> +            local idx, path = format_field_resolve(format, part.field)
> +            if idx == nil then
> +               box.error(box.error.ILLEGAL_PARAMS,

Bad formatting - should be 4 spaces after if, not 3.

> +                         "options.parts[" .. i .. "]: field was not found by name '" .. part.field .. "'")

The line is too long. Please split.

>              end
> -            if type(part.field) == 'string' then
> +            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 '"..
> +                          path.." doesn't math the path '" ..part.path.."'")

Sometimes you add spaces around concatenation operator (..), sometimes
you don't. I think I've already repeated it a dozen times, but let me
try again: please be consistent throughout the code.

Anyway, I don't think that there's any point to ensure that part.path
matches the path extracted from the field. The error message looks weird
too. Let's just ignore part.path in this case.

>              end
> +            parts_can_be_simplified = parts_can_be_simplified and path == nil
> +            part.field = idx
> +            part.path = path or part.path
>          elseif part.field == 0 then
>              box.error(box.error.ILLEGAL_PARAMS,
>                        "options.parts[" .. i .. "]: field (number) must be one-based")

Let's please unite all the code doing field resolution in
tuple_field_resolve, including the case when the field is
given as a number. And check that the field is 1-based in
all cases.

> diff --git a/test/engine/json.result b/test/engine/json.result
> index 9e5accb0b..32f787a3d 100644
> --- a/test/engine/json.result
> +++ b/test/engine/json.result
> @@ -70,6 +70,48 @@ s:create_index('test2', {parts = {{2, 'number'}, {3, 'number', path = 'FIO.fname
>  - error: Field [3]["FIO"]["fname"] has type 'string' in one index, but type 'number'
>      in another
>  ...
> +s:create_index('test3', {parts = {{2, 'number'}, {']sad.FIO["fname"]', 'str'}}})
> +---
> +- error: 'Illegal parameters, options.parts[2]: field was not found by name '']sad.FIO["fname"]'''
> +...
> +s:create_index('test3', {parts = {{2, 'number'}, {'[3].FIO["fname"]', 'str', path = "FIO[\"sname\"]"}}})
> +---
> +- error: 'Illegal parameters, options.parts[2]: field path ''.FIO["fname"] doesn''t
> +    math the path ''FIO["sname"]'''
> +...
> +idx3 = s:create_index('test3', {parts = {{2, 'number'}, {'[3].FIO["fname"]', 'str'}}})
> +---
> +...
> +idx3 ~= nil
> +---
> +- true
> +...
> +idx3.parts[2].path == ".FIO[\"fname\"]"
> +---
> +- true
> +...
> +s:create_index('test4', {parts = {{2, 'number'}, {'invalid.FIO["fname"]', 'str'}}})
> +---
> +- error: 'Illegal parameters, options.parts[2]: field was not found by name ''invalid.FIO["fname"]'''
> +...
> +idx4 = s:create_index('test4', {parts = {{2, 'number'}, {'data.FIO["fname"]', 'str'}}})
> +---
> +...
> +idx4 ~= nil
> +---
> +- true
> +...
> +idx4.parts[2].path == ".FIO[\"fname\"]"
> +---
> +- true
> +...
> +-- Vinyl has optimizations that omit index checks, so errors could differ.

What's this about? I don't understand...

> +idx3:drop()
> +---
> +...
> +idx4:drop()
> +---
> +...
>  s:insert{7, 7, {town = 'London', FIO = 666}, 4, 5}
>  ---
>  - error: 'Tuple field [3]["FIO"] type does not match one required by operation: expected

I'm not planning to review the next version of this patch set for a week
or two so please take your time and perfect it finally unless you want
it to be delayed for another month.