From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 4 Dec 2018 15:22:39 +0300 From: Vladimir Davydov Subject: Re: [PATCH v5 9/9] box: specify indexes in user-friendly form Message-ID: <20181204122239.hs5qy2ojvb56prs2@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org, kostja@tarantool.org List-ID: 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)