Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org, kostja@tarantool.org
Subject: Re: [PATCH v5 9/9] box: specify indexes in user-friendly form
Date: Tue, 4 Dec 2018 15:22:39 +0300	[thread overview]
Message-ID: <20181204122239.hs5qy2ojvb56prs2@esperanza> (raw)
In-Reply-To: <d8c6fe8028944a7d5b198eb18ca21e7f6392fd07.1543229303.git.kshcherbatov@tarantool.org>

On Mon, Nov 26, 2018 at 01:49:43PM +0300, Kirill Shcherbatov wrote:
> It is now possible to create indexes by JSON path and using
> field names specified in the space format.
> 
> Closes #1012
> 
> @TarantoolBot document
> Title: Indexes by JSON path
> Sometimes field data could have complex document structure.
> When this structure is consistent across whole document,

whole document => whole space

> you are able to create an index by JSON path.
> 
> Example:
> s:create_index('json_index', {parts = {{'FIO["fname"]', 'str'}}})

Please describe an alternative way of creating an index (via path).


> @@ -626,16 +626,16 @@ 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
> -            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

I'd prefer if we didn't involve a space in this function: currently
update_index_parts is a neat independent function that takes a format
and does some index part transformations. After your patch it requires
both space_id and format. This looks like encapsulation violation to me.
It would be great if we could rewrite it without it.

After all, all we need to do is extract the first component of a json
path and I think this should be fairly easy to implement in Lua.

>                  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 the path '" ..

math => match

Anyway, I think that you should raise an error unconditionally if the
user tries to pass 'path' without a field number. Also, this error path
isn't covered by the test. Please fix.

> +                          part.field .. "'")
>              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")
> @@ -792,7 +792,7 @@ box.schema.index.create = function(space_id, name, options)
>          end
>      end
>      local parts, parts_can_be_simplified =
> -        update_index_parts(format, options.parts)
> +        update_index_parts(format, options.parts, space_id)
>      -- create_index() options contains type, parts, etc,
>      -- stored separately. Remove these members from index_opts
>      local index_opts = {
> @@ -959,7 +959,7 @@ box.schema.index.alter = function(space_id, index_id, options)
>      if options.parts then
>          local parts_can_be_simplified
>          parts, parts_can_be_simplified =
> -            update_index_parts(format, options.parts)
> +            update_index_parts(format, options.parts, space_id)
>          -- save parts in old format if possible
>          if parts_can_be_simplified then
>              parts = simplify_index_parts(parts)

      reply	other threads:[~2018-12-04 12:22 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-26 10:49 [PATCH v5 0/9] box: indexes by JSON path Kirill Shcherbatov
2018-11-26 10:49 ` [PATCH v5 1/9] box: refactor json_path_parser class Kirill Shcherbatov
2018-11-26 12:53   ` [tarantool-patches] " Kirill Shcherbatov
2018-11-29 15:39     ` Vladimir Davydov
2018-11-26 10:49 ` [PATCH v5 2/9] lib: implement JSON tree class for json library Kirill Shcherbatov
2018-11-26 12:53   ` [tarantool-patches] " Kirill Shcherbatov
2018-11-29 17:38     ` Vladimir Davydov
2018-11-29 17:50       ` Vladimir Davydov
2018-12-04 15:22       ` Vladimir Davydov
2018-12-04 15:47       ` [tarantool-patches] " Kirill Shcherbatov
2018-12-04 17:54         ` Vladimir Davydov
2018-12-05  8:37           ` Kirill Shcherbatov
2018-12-05  9:07             ` Vladimir Davydov
2018-12-05  9:52               ` Vladimir Davydov
2018-12-06  7:56                 ` Kirill Shcherbatov
2018-12-06  7:56                 ` [tarantool-patches] Re: [PATCH v5 2/9] lib: make index_base support for json_lexer Kirill Shcherbatov
2018-11-26 10:49 ` [PATCH v5 3/9] box: manage format fields with JSON tree class Kirill Shcherbatov
2018-11-29 19:07   ` Vladimir Davydov
2018-12-04 15:47     ` [tarantool-patches] " Kirill Shcherbatov
2018-12-04 16:09       ` Vladimir Davydov
2018-12-04 16:32         ` Kirill Shcherbatov
2018-12-05  8:37         ` Kirill Shcherbatov
2018-12-06  7:56         ` Kirill Shcherbatov
2018-12-06  8:06           ` Vladimir Davydov
2018-11-26 10:49 ` [PATCH v5 4/9] lib: introduce json_path_cmp routine Kirill Shcherbatov
2018-11-30 10:46   ` Vladimir Davydov
2018-12-03 17:37     ` [tarantool-patches] " Konstantin Osipov
2018-12-03 18:48       ` Vladimir Davydov
2018-12-03 20:14         ` Konstantin Osipov
2018-12-06  7:56           ` [tarantool-patches] Re: [PATCH v5 4/9] lib: introduce json_path_cmp, json_path_validate Kirill Shcherbatov
2018-11-26 10:49 ` [tarantool-patches] [PATCH v5 5/9] box: introduce JSON indexes Kirill Shcherbatov
2018-11-30 21:28   ` Vladimir Davydov
2018-12-01 16:49     ` Vladimir Davydov
2018-11-26 10:49 ` [PATCH v5 6/9] box: introduce has_json_paths flag in templates Kirill Shcherbatov
2018-11-26 10:49 ` [PATCH v5 7/9] box: tune tuple_field_raw_by_path for indexed data Kirill Shcherbatov
2018-12-01 17:20   ` Vladimir Davydov
2018-11-26 10:49 ` [PATCH v5 8/9] box: introduce offset slot cache in key_part Kirill Shcherbatov
2018-12-03 21:04   ` Vladimir Davydov
2018-12-04 15:51     ` Vladimir Davydov
2018-11-26 10:49 ` [PATCH v5 9/9] box: specify indexes in user-friendly form Kirill Shcherbatov
2018-12-04 12:22   ` Vladimir Davydov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181204122239.hs5qy2ojvb56prs2@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kostja@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [PATCH v5 9/9] box: specify indexes in user-friendly form' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox