From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 23 Jan 2019 18:29:03 +0300 From: Vladimir Davydov Subject: Re: [PATCH v8 5/5] box: specify indexes in user-friendly form Message-ID: <20190123152903.27pfxg4hyg4oyjex@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 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.