From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 8617E469710 for ; Sat, 21 Nov 2020 22:07:44 +0300 (MSK) From: Oleg Babin References: Message-ID: <52439555-a382-189e-941a-22aa4b0e2683@tarantool.org> Date: Sat, 21 Nov 2020 22:07:42 +0300 MIME-Version: 1.0 In-Reply-To: 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: Vladislav Shpilevoy , lvasiliev@tarantool.org Cc: tarantool-patches@dev.tarantool.org Thanks for your comments. I've force-pushed fixes to my branch (https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2). See my answers below. On 21/11/2020 18:17, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 3 comments below. > > On 18.11.2020 08:56,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 > 1. vaues -> values. Thanks! Fixed. Consider a new commit message. I moved docbot request to the next patch. ```     uuid: support comparison of uuid values in Lua     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 ``` >> 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/exports.h b/src/exports.h >> index 867a027dc..ffbb84e3b 100644 >> --- a/src/exports.h >> +++ b/src/exports.h >> @@ -508,6 +508,7 @@ EXPORT(tnt_iconv_close) >> EXPORT(tnt_iconv_open) >> EXPORT(tt_uuid_bswap) >> EXPORT(tt_uuid_create) >> +EXPORT(tt_uuid_compare) > 2. Please, keep the lines sorted, as it is asked in the > first lines of this file. 'r' > 'o', so tt_uuid_compare > should be before tt_uuid_create. My bad. Fixed. >> EXPORT(tt_uuid_from_string) >> EXPORT(tt_uuid_is_equal) >> EXPORT(tt_uuid_is_nil) >> 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 >> @@ -118,9 +120,32 @@ local uuid_new_str = function() >> return uuid_tostring(uuidbuf) >> end >> >> +local check_uuid = function(value, index) >> + if is_uuid(value) then >> + return value >> + end >> + >> + local err_fmt = 'incorrect value to compare with uuid as %d argument' >> + error(err_fmt:format(index), 0) > 3. Why did you use 0? You loose the error location then. > I changed it to 4, and got more informative messages, showing > the error location: > > [001] 'abc' <= u1 > [001] --- > [001] -- error: incorrect value to compare with uuid as 1 argument > [001] +- error: '[string "return ''abc'' <= u1 "]:1: incorrect value to compare with uuid > [001] + as 1 argument' > > I also noticed just now, that we already have uuid_eq, which looks > different. I think you should unify the comparators, while you are > here. Otherwise eq has its own 'check_uuid', and here you add a second > one. It is inconsistent, even error messages look different. Also you > anyway change it in the next commit, so it should not increase diff > much. Agree. Consider new diff diff --git a/src/exports.h b/src/exports.h index ffbb84e3b..2116036fa 100644 --- a/src/exports.h +++ b/src/exports.h @@ -507,8 +507,8 @@ EXPORT(tnt_iconv)  EXPORT(tnt_iconv_close)  EXPORT(tnt_iconv_open)  EXPORT(tt_uuid_bswap) -EXPORT(tt_uuid_create)  EXPORT(tt_uuid_compare) +EXPORT(tt_uuid_create)  EXPORT(tt_uuid_from_string)  EXPORT(tt_uuid_is_equal)  EXPORT(tt_uuid_is_nil) diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua index 08991cfeb..275f4e5c5 100644 --- a/src/lua/uuid.lua +++ b/src/lua/uuid.lua @@ -93,13 +93,20 @@ local uuid_isnil = function(uu)      return builtin.tt_uuid_is_nil(uu)  end +local check_uuid = function(value, index) +    if is_uuid(value) then +        return value +    end + +    local err_fmt = 'incorrect value to compare with uuid as %d argument' +    error(err_fmt:format(index), 4) +end +  local uuid_eq = function(lhs, rhs)      if not is_uuid(rhs) then          return false      end -    if not is_uuid(lhs) then -        return error('Usage: uuid == var') -    end +    lhs = check_uuid(lhs, 1)      return builtin.tt_uuid_is_equal(lhs, rhs)  end @@ -120,15 +127,6 @@ local uuid_new_str = function()      return uuid_tostring(uuidbuf)  end -local check_uuid = function(value, index) -    if is_uuid(value) then -        return value -    end - -    local err_fmt = 'incorrect value to compare with uuid as %d argument' -    error(err_fmt:format(index), 0) -end -  local uuid_cmp = function(lhs, rhs)      lhs = check_uuid(lhs, 1)      rhs = check_uuid(rhs, 2) diff --git a/test/app/uuid.result b/test/app/uuid.result index e06331001..b70cb0e32 100644 --- a/test/app/uuid.result +++ b/test/app/uuid.result @@ -334,35 +334,41 @@ u1 < u2  ...  u1 < 1  --- -- error: incorrect value to compare with uuid as 2 argument +- error: '[string "return u1 < 1 "]:1: incorrect value to compare with uuid as 2 argument'  ...  u1 <= 1  --- -- error: incorrect value to compare with uuid as 2 argument +- error: '[string "return u1 <= 1 "]:1: incorrect value to compare with uuid as 2 +    argument'  ...  u1 < 'abc'  --- -- error: incorrect value to compare with uuid as 2 argument +- error: '[string "return u1 < ''abc'' "]:1: incorrect value to compare with uuid +    as 2 argument'  ...  u1 <= 'abc'  --- -- error: incorrect value to compare with uuid as 2 argument +- error: '[string "return u1 <= ''abc'' "]:1: incorrect value to compare with uuid +    as 2 argument'  ...  1 < u1  --- -- error: incorrect value to compare with uuid as 1 argument +- error: '[string "return 1 < u1 "]:1: incorrect value to compare with uuid as 1 argument'  ...  1 <= u1  --- -- error: incorrect value to compare with uuid as 1 argument +- error: '[string "return 1 <= u1 "]:1: incorrect value to compare with uuid as 1 +    argument'  ...  'abc' < u1  --- -- error: incorrect value to compare with uuid as 1 argument +- error: '[string "return ''abc'' < u1 "]:1: incorrect value to compare with uuid +    as 1 argument'  ...  'abc' <= u1  --- -- error: incorrect value to compare with uuid as 1 argument +- error: '[string "return ''abc'' <= u1 "]:1: incorrect value to compare with uuid +    as 1 argument'  ...  u1 = nil  ---