From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] Re: [PATCH 4/8] netbox: extend codec with 'decode' methods References: <5196d19705a653bfbf600cf787e05f4f75df99f6.1523903144.git.v.shpilevoy@tarantool.org> <20180423164227.s4rgh7zg4qzojk2k@esperanza> From: Vladislav Shpilevoy Message-ID: Date: Mon, 23 Apr 2018 21:59:13 +0300 MIME-Version: 1.0 In-Reply-To: <20180423164227.s4rgh7zg4qzojk2k@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: Hello. Thanks for review! See the diff at the end of letter. >> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua >> index 4ed2b375d..3868cdf1c 100644 >> --- a/src/box/lua/net_box.lua >> +++ b/src/box/lua/net_box.lua >> @@ -50,7 +50,34 @@ local E_PROC_LUA = box.error.PROC_LUA >> >> -- utility tables >> local is_final_state = {closed = 1, error = 1} >> -local method_codec = { >> + >> +local function decode_nil(...) end > >> +local function decode_nothing(...) return ... end > > decode_nop, may be? Done. > >> +local function decode_one_tuple(response) >> + if response[1] then >> + return box.tuple.new(response[1]) >> + end >> +end >> +local function decode_single_tuple(response) >> + if response[2] then >> + return nil, box.error.MORE_THAN_ONE_TUPLE >> + end >> + if response[1] then >> + return box.tuple.new(response[1]) >> + end >> +end > > Do we really need this MORE_THAN_ONE_TUPLE error? Can't we use > decode_one_tuple in 'get' as well? I ask, because having two methods > called decode_one_tuple and decode_single_tuple is confusing IMO - > it's unclear what's the difference between them. I agree with you, it looks ugly, but IProto has no 'get' method, so we must emulate it via select(limit = 2) + check that a tuple is only one. I renamed decode_single to decode_unique. Maybe it looks not so bad. > >> +local function decode_select(response) >> + setmetatable(response, sequence_mt) >> + for i, v in pairs(response) do >> + response[i] = box.tuple.new(v) >> + end >> + return response >> +end >> +local function decode_count(response) >> + return response[1] >> +end >> + >> +local method_encoder = { >> ping = internal.encode_ping, >> call_16 = internal.encode_call_16, >> call_17 = internal.encode_call, >> @@ -61,6 +88,10 @@ local method_codec = { >> update = internal.encode_update, >> upsert = internal.encode_upsert, >> select = internal.encode_select, >> + get = internal.encode_select, >> + min = internal.encode_select, >> + max = internal.encode_select, >> + count = internal.encode_call, >> -- inject raw data into connection, used by console and tests >> inject = function(buf, id, schema_version, bytes) >> local ptr = buf:reserve(#bytes) >> @@ -69,6 +100,24 @@ local method_codec = { >> end >> } >> >> +local method_decoder = { >> + ping = decode_nil, >> + call_16 = decode_select, >> + call_17 = decode_nothing, >> + eval = decode_nothing, >> + insert = decode_one_tuple, >> + replace = decode_one_tuple, >> + delete = decode_one_tuple, >> + update = decode_one_tuple, >> + upsert = decode_nil, >> + select = decode_select, >> + get = decode_single_tuple, >> + min = decode_single_tuple, >> + max = decode_single_tuple, >> + count = decode_count, >> + inject = decode_nothing, >> +} >> + >> local function next_id(id) return band(id + 1, 0x7FFFFFFF) end >> >> -- > >> @@ -1144,9 +1175,8 @@ index_metatable = function(remote) >> if opts and opts.buffer then >> error("index:min() doesn't support `buffer` argument") >> end >> - local res = remote:_request('select', opts, self.space.id, self.id, >> - box.index.GE, 0, 1, key) >> - return one_tuple(res) >> + return remote:_request('get', opts, self.space.id, self.id, > > Should be 'min'? Yes, did not notice that. Fixed on the branch. > >> + box.index.GE, 0, 1, key) >> end >> >> function methods:max(key, opts) >> @@ -1154,9 +1184,8 @@ index_metatable = function(remote) >> if opts and opts.buffer then >> error("index:max() doesn't support `buffer` argument") >> end >> - local res = remote:_request('select', opts, self.space.id, self.id, >> - box.index.LE, 0, 1, key) >> - return one_tuple(res) >> + return remote:_request('get', opts, self.space.id, self.id, > > Should be 'max'? Same, you are right. Fixed. See the diff: diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua index 3868cdf1c..53f301356 100644 --- a/src/box/lua/net_box.lua +++ b/src/box/lua/net_box.lua @@ -52,13 +52,13 @@ local E_PROC_LUA = box.error.PROC_LUA local is_final_state = {closed = 1, error = 1} local function decode_nil(...) end -local function decode_nothing(...) return ... end +local function decode_nop(...) return ... end local function decode_one_tuple(response) if response[1] then return box.tuple.new(response[1]) end end -local function decode_single_tuple(response) +local function decode_unique_tuple(response) if response[2] then return nil, box.error.MORE_THAN_ONE_TUPLE end @@ -103,19 +103,19 @@ local method_encoder = { local method_decoder = { ping = decode_nil, call_16 = decode_select, - call_17 = decode_nothing, - eval = decode_nothing, + call_17 = decode_nop, + eval = decode_nop, insert = decode_one_tuple, replace = decode_one_tuple, delete = decode_one_tuple, update = decode_one_tuple, upsert = decode_nil, select = decode_select, - get = decode_single_tuple, - min = decode_single_tuple, - max = decode_single_tuple, + get = decode_unique_tuple, + min = decode_unique_tuple, + max = decode_unique_tuple, count = decode_count, - inject = decode_nothing, + inject = decode_nop, } local function next_id(id) return band(id + 1, 0x7FFFFFFF) end @@ -1175,7 +1175,7 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:min() doesn't support `buffer` argument") end - return remote:_request('get', opts, self.space.id, self.id, + return remote:_request('min', opts, self.space.id, self.id, box.index.GE, 0, 1, key) end @@ -1184,7 +1184,7 @@ index_metatable = function(remote) if opts and opts.buffer then error("index:max() doesn't support `buffer` argument") end - return remote:_request('get', opts, self.space.id, self.id, + return remote:_request('max', opts, self.space.id, self.id, box.index.LE, 0, 1, key) end