From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp30.i.mail.ru (smtp30.i.mail.ru [94.100.177.90]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 78EBE469710 for ; Tue, 24 Nov 2020 22:23:09 +0300 (MSK) From: Oleg Babin References: <20201124152043.GE14086@tarantool.org> Message-ID: <73a9f4a4-c1c1-84c5-c465-1986a3adbff8@tarantool.org> Date: Tue, 24 Nov 2020 22:23:08 +0300 MIME-Version: 1.0 In-Reply-To: <20201124152043.GE14086@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Munkin Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org Thanks for your review! On 24/11/2020 18:20, Igor Munkin wrote: > Oleg, > > Thanks for the patch! Considering the version on the remote branch, LGTM > except a single nit below. > > On 18.11.20,olegrok@tarantool.org wrote: >> From: Oleg Babin >> >> Since Tarantool has uuid data type sometimes we want to compare >> uuid vaues as it's possible for primitive types and decimals. This >> patch exports function for uuid comparison and implements le and >> lt metamethods for uuid type. > Minor: Strictly saying, you're right, but AFAICS we usually use the key > name (i.e. __le and __lt) for metamethods in description. Fixed. Description is updated, branch is force-pushed. New description:     Since Tarantool has uuid data type sometimes we want to compare     uuid values as it's possible for primitive types and decimals. This     patch exports function for uuid comparison and implements __le and     __lt metamethods for uuid type. >> Closes #5511 >> >> @TarantoolBot document >> Title: uuid comparison is supported >> >> Currently comparison between uuid values is supported. >> Example: >> ```lua >> u1 = uuid.fromstr('aaaaaaaa-aaaa-4000-b000-000000000001') >> u2 = uuid.fromstr('bbbbbbbb-bbbb-4000-b000-000000000001') >> >> u1 > u2 -- false >> u1 >= u2 -- false >> u1 <= u2 -- true >> u1 < u2 -- true >> ``` >> --- >> Issue:https://github.com/tarantool/tarantool/issues/5511 >> Branch:https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2 >> >> src/exports.h | 1 + >> src/lua/uuid.lua | 25 +++++++++++++ >> test/app/uuid.result | 79 ++++++++++++++++++++++++++++++++++++++++++ >> test/app/uuid.test.lua | 29 ++++++++++++++++ >> 4 files changed, 134 insertions(+) >> > > >> diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua >> index 42016601d..08991cfeb 100644 >> --- a/src/lua/uuid.lua >> +++ b/src/lua/uuid.lua > > >> +local uuid_lt = function(lhs, rhs) >> + return uuid_cmp(lhs, rhs) < 0 >> +end >> +local uuid_le = function(lhs, rhs) >> + return uuid_cmp(lhs, rhs) <= 0 >> +end > Side note: I see no reason to define these oneline functions (you can > simply store an anonymous function right to the metatable), but I see > them everywhere in Tarantool sources, so feel free to ignore this note. Let's keep general style of this module and define comparators as local functions. I stay them as is.