Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable
@ 2020-11-18  7:56 olegrok
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua olegrok
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: olegrok @ 2020-11-18  7:56 UTC (permalink / raw)
  To: v.shpilevoy, lvasiliev; +Cc: tarantool-patches

From: Oleg Babin <babinoleg@mail.ru>

This patchset makes uuid values comparable.

The first one allows
to compare only uuid values. It's just define lt and le methods
for uuid values.

The second one allows to compare uuid values with string
representations of uuid. Just note that this patch breaks backward
compatibility - before all attempts to check equality with
non-uuid values returned false. Currently it's not so.
It we want do that this patch could be omitted.

Issue: https://github.com/tarantool/tarantool/issues/5511
Branch: https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2

Changes with v1:
  - Use static_alloc when string is converted to uuid to compare
  - Style fixes
  - Extend "eq" for comparison with string
  - Split patch in two parts

Oleg Babin (2):
  uuid: support comparison of uuid values in Lua
  uuid: support uuid comparison with strings

 src/exports.h          |   1 +
 src/lua/uuid.lua       |  52 ++++++++++++++-
 test/app/uuid.result   | 148 +++++++++++++++++++++++++++++++++++++++++
 test/app/uuid.test.lua |  56 ++++++++++++++++
 4 files changed, 255 insertions(+), 2 deletions(-)

-- 
2.29.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-18  7:56 [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable olegrok
@ 2020-11-18  7:56 ` olegrok
  2020-11-21 15:17   ` Vladislav Shpilevoy
  2020-11-24 15:20   ` Igor Munkin
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings olegrok
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: olegrok @ 2020-11-18  7:56 UTC (permalink / raw)
  To: v.shpilevoy, lvasiliev; +Cc: tarantool-patches

From: Oleg Babin <babinoleg@mail.ru>

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.

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)
 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
@@ -19,6 +19,8 @@ bool
 tt_uuid_is_equal(const struct tt_uuid *lhs, const struct tt_uuid *rhs);
 char *
 tt_uuid_str(const struct tt_uuid *uu);
+int
+tt_uuid_compare(const struct tt_uuid *a, const struct tt_uuid *b);
 extern const struct tt_uuid uuid_nil;
 ]]
 
@@ -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)
+end
+
+local uuid_cmp = function(lhs, rhs)
+    lhs = check_uuid(lhs, 1)
+    rhs = check_uuid(rhs, 2)
+    return builtin.tt_uuid_compare(lhs, rhs)
+end
+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
+
 local uuid_mt = {
     __tostring = uuid_tostring;
     __eq = uuid_eq;
+    __lt = uuid_lt;
+    __le = uuid_le;
     __index = {
         isnil = uuid_isnil;
         bin   = uuid_tobin;    -- binary host byteorder
diff --git a/test/app/uuid.result b/test/app/uuid.result
index 9fe0e7fb4..e06331001 100644
--- a/test/app/uuid.result
+++ b/test/app/uuid.result
@@ -291,6 +291,85 @@ uuid.is_uuid(require('decimal').new('123'))
 ---
 - false
 ...
+--
+-- gh-5511: allow to compare uuid values
+--
+u1 = uuid.fromstr('aaaaaaaa-aaaa-4000-b000-000000000001')
+---
+...
+u2 = uuid.fromstr('bbbbbbbb-bbbb-4000-b000-000000000001')
+---
+...
+u1 > u1
+---
+- false
+...
+u1 >= u1
+---
+- true
+...
+u1 <= u1
+---
+- true
+...
+u1 < u1
+---
+- false
+...
+u1 > u2
+---
+- false
+...
+u1 >= u2
+---
+- false
+...
+u1 <= u2
+---
+- true
+...
+u1 < u2
+---
+- true
+...
+u1 < 1
+---
+- error: incorrect value to compare with uuid as 2 argument
+...
+u1 <= 1
+---
+- error: incorrect value to compare with uuid as 2 argument
+...
+u1 < 'abc'
+---
+- error: incorrect value to compare with uuid as 2 argument
+...
+u1 <= 'abc'
+---
+- error: incorrect value to compare with uuid as 2 argument
+...
+1 < u1
+---
+- error: incorrect value to compare with uuid as 1 argument
+...
+1 <= u1
+---
+- error: incorrect value to compare with uuid as 1 argument
+...
+'abc' < u1
+---
+- error: incorrect value to compare with uuid as 1 argument
+...
+'abc' <= u1
+---
+- error: incorrect value to compare with uuid as 1 argument
+...
+u1 = nil
+---
+...
+u2 = nil
+---
+...
 uuid = nil
 ---
 ...
diff --git a/test/app/uuid.test.lua b/test/app/uuid.test.lua
index 47a96f3c6..34ab38d35 100644
--- a/test/app/uuid.test.lua
+++ b/test/app/uuid.test.lua
@@ -108,6 +108,35 @@ uuid.is_uuid(uuid.new():str())
 uuid.is_uuid(1)
 uuid.is_uuid(require('decimal').new('123'))
 
+--
+-- gh-5511: allow to compare uuid values
+--
+
+u1 = uuid.fromstr('aaaaaaaa-aaaa-4000-b000-000000000001')
+u2 = uuid.fromstr('bbbbbbbb-bbbb-4000-b000-000000000001')
+
+u1 > u1
+u1 >= u1
+u1 <= u1
+u1 < u1
+
+u1 > u2
+u1 >= u2
+u1 <= u2
+u1 < u2
+
+u1 < 1
+u1 <= 1
+u1 < 'abc'
+u1 <= 'abc'
+1 < u1
+1 <= u1
+'abc' < u1
+'abc' <= u1
+
+u1 = nil
+u2 = nil
+
 uuid = nil
 
 test_run:cmd("clear filter")
-- 
2.29.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings
  2020-11-18  7:56 [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable olegrok
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua olegrok
@ 2020-11-18  7:56 ` olegrok
  2020-11-21 15:17   ` Vladislav Shpilevoy
  2020-11-24 15:20   ` Igor Munkin
  2020-11-18  8:02 ` [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable Oleg Babin
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 21+ messages in thread
From: olegrok @ 2020-11-18  7:56 UTC (permalink / raw)
  To: v.shpilevoy, lvasiliev; +Cc: tarantool-patches

From: Oleg Babin <babinoleg@mail.ru>

Before this patch it was impossible to compare uuid values with
string representations of uuid. However we have cases when such
comparisons is possible (e.g. "decimal" where we can compare
decimal values with strings and numbers).

This patch extends uuid comparators (eq, lt, le) and every string
argument is tried to be converted to uuid value to compare then.

Follow-up #5511

@TarantoolBot document
Title: uuid values could be compared with strings

Currently it's possible to compare uuid values with its string
representations:
```lua
u1_str = 'aaaaaaaa-aaaa-4000-b000-000000000001'
u1 = uuid.fromstr(u1_str)
u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'

u1 == u1_str -- true
u1 == u2_str -- false

u1 >= u1_str -- true
u1 < u2_str  -- true
```
---
Issue: https://github.com/tarantool/tarantool/issues/5511
Branch: https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2

 src/lua/uuid.lua       | 31 +++++++++++++--
 test/app/uuid.result   | 85 ++++++++++++++++++++++++++++++++++++++----
 test/app/uuid.test.lua | 27 ++++++++++++++
 3 files changed, 131 insertions(+), 12 deletions(-)

diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
index 08991cfeb..1d5d835df 100644
--- a/src/lua/uuid.lua
+++ b/src/lua/uuid.lua
@@ -93,11 +93,33 @@ local uuid_isnil = function(uu)
     return builtin.tt_uuid_is_nil(uu)
 end
 
+local fromstr = function(str)
+    local uu = static_alloc('struct tt_uuid')
+    local rc = builtin.tt_uuid_from_string(str, uu)
+    if rc ~= 0 then
+        return nil
+    end
+    return uu
+end
+
+local to_uuid = function(value)
+    if is_uuid(value) then
+        return value
+    end
+    if type(value) == 'string' then
+        return fromstr(value)
+    end
+    return nil
+end
+
 local uuid_eq = function(lhs, rhs)
-    if not is_uuid(rhs) then
+    rhs = to_uuid(rhs)
+    if rhs == nil then
         return false
     end
-    if not is_uuid(lhs) then
+
+    lhs = to_uuid(lhs)
+    if lhs == nil then
         return error('Usage: uuid == var')
     end
     return builtin.tt_uuid_is_equal(lhs, rhs)
@@ -121,11 +143,12 @@ local uuid_new_str = function()
 end
 
 local check_uuid = function(value, index)
-    if is_uuid(value) then
+    value = to_uuid(value)
+    if value ~= nil then
         return value
     end
 
-    local err_fmt = 'incorrect value to compare with uuid as %d argument'
+    local err_fmt = 'incorrect value to convert to uuid as %d argument'
     error(err_fmt:format(index), 0)
 end
 
diff --git a/test/app/uuid.result b/test/app/uuid.result
index e06331001..5b9ffa230 100644
--- a/test/app/uuid.result
+++ b/test/app/uuid.result
@@ -334,35 +334,35 @@ u1 < u2
 ...
 u1 < 1
 ---
-- error: incorrect value to compare with uuid as 2 argument
+- error: incorrect value to convert to uuid as 2 argument
 ...
 u1 <= 1
 ---
-- error: incorrect value to compare with uuid as 2 argument
+- error: incorrect value to convert to uuid as 2 argument
 ...
 u1 < 'abc'
 ---
-- error: incorrect value to compare with uuid as 2 argument
+- error: incorrect value to convert to uuid as 2 argument
 ...
 u1 <= 'abc'
 ---
-- error: incorrect value to compare with uuid as 2 argument
+- error: incorrect value to convert to uuid as 2 argument
 ...
 1 < u1
 ---
-- error: incorrect value to compare with uuid as 1 argument
+- error: incorrect value to convert to uuid as 1 argument
 ...
 1 <= u1
 ---
-- error: incorrect value to compare with uuid as 1 argument
+- error: incorrect value to convert to uuid as 1 argument
 ...
 'abc' < u1
 ---
-- error: incorrect value to compare with uuid as 1 argument
+- error: incorrect value to convert to uuid as 1 argument
 ...
 'abc' <= u1
 ---
-- error: incorrect value to compare with uuid as 1 argument
+- error: incorrect value to convert to uuid as 1 argument
 ...
 u1 = nil
 ---
@@ -370,6 +370,75 @@ u1 = nil
 u2 = nil
 ---
 ...
+--
+-- allow to compare uuid values with strings
+--
+u1_str = 'aaaaaaaa-aaaa-4000-b000-000000000001'
+---
+...
+u1 = uuid.fromstr(u1_str)
+---
+...
+u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'
+---
+...
+u1 == u1_str
+---
+- true
+...
+u1 == u2_str
+---
+- false
+...
+u1_str == u1
+---
+- true
+...
+u2_str == u1
+---
+- false
+...
+u1 > u1_str
+---
+- false
+...
+u1 >= u1_str
+---
+- true
+...
+u1 < u1_str
+---
+- false
+...
+u1 <= u1_str
+---
+- true
+...
+u1 > u2_str
+---
+- false
+...
+u1 >= u2_str
+---
+- false
+...
+u1 < u2_str
+---
+- true
+...
+u1 <= u2_str
+---
+- true
+...
+u1 = nil
+---
+...
+u1_str = nil
+---
+...
+u2_str = nil
+---
+...
 uuid = nil
 ---
 ...
diff --git a/test/app/uuid.test.lua b/test/app/uuid.test.lua
index 34ab38d35..867bbd832 100644
--- a/test/app/uuid.test.lua
+++ b/test/app/uuid.test.lua
@@ -137,6 +137,33 @@ u1 <= 'abc'
 u1 = nil
 u2 = nil
 
+--
+-- allow to compare uuid values with strings
+--
+
+u1_str = 'aaaaaaaa-aaaa-4000-b000-000000000001'
+u1 = uuid.fromstr(u1_str)
+u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'
+
+u1 == u1_str
+u1 == u2_str
+u1_str == u1
+u2_str == u1
+
+u1 > u1_str
+u1 >= u1_str
+u1 < u1_str
+u1 <= u1_str
+
+u1 > u2_str
+u1 >= u2_str
+u1 < u2_str
+u1 <= u2_str
+
+u1 = nil
+u1_str = nil
+u2_str = nil
+
 uuid = nil
 
 test_run:cmd("clear filter")
-- 
2.29.0

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable
  2020-11-18  7:56 [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable olegrok
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua olegrok
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings olegrok
@ 2020-11-18  8:02 ` Oleg Babin
  2020-11-27 22:39 ` Vladislav Shpilevoy
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Oleg Babin @ 2020-11-18  8:02 UTC (permalink / raw)
  To: v.shpilevoy, lvasiliev; +Cc: tarantool-patches

@Changelog:
   - Allow to compare uuid values (gh-5511)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua olegrok
@ 2020-11-21 15:17   ` Vladislav Shpilevoy
  2020-11-21 19:07     ` Oleg Babin
  2020-11-24 15:20   ` Igor Munkin
  1 sibling, 1 reply; 21+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-21 15:17 UTC (permalink / raw)
  To: olegrok, lvasiliev; +Cc: tarantool-patches

Hi! Thanks for the patch!

See 3 comments below.

On 18.11.2020 08:56, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> 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.

> 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.

>  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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings olegrok
@ 2020-11-21 15:17   ` Vladislav Shpilevoy
  2020-11-21 19:07     ` Oleg Babin
  2020-11-24 15:20   ` Igor Munkin
  1 sibling, 1 reply; 21+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-21 15:17 UTC (permalink / raw)
  To: olegrok, lvasiliev; +Cc: tarantool-patches

Thanks for the patch!

See 2 comments below.

On 18.11.2020 08:56, olegrok@tarantool.org wrote:
> From: Oleg Babin <babinoleg@mail.ru>
> 
> Before this patch it was impossible to compare uuid values with
> string representations of uuid. However we have cases when such
> comparisons is possible (e.g. "decimal" where we can compare
> decimal values with strings and numbers).
> 
> This patch extends uuid comparators (eq, lt, le) and every string
> argument is tried to be converted to uuid value to compare then.
> 
> Follow-up #5511
> 
> @TarantoolBot document
> Title: uuid values could be compared with strings
> 
> Currently it's possible to compare uuid values with its string
> representations:
> ```lua
> u1_str = 'aaaaaaaa-aaaa-4000-b000-000000000001'
> u1 = uuid.fromstr(u1_str)
> u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'
> 
> u1 == u1_str -- true
> u1 == u2_str -- false
> 
> u1 >= u1_str -- true
> u1 < u2_str  -- true
> ```

1. Better use a single docbot request. It will be simpler for the doc
team to handle it, since both the updates are about the same place.
But up to you.

> ---
> Issue: https://github.com/tarantool/tarantool/issues/5511
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2
> 
> diff --git a/test/app/uuid.result b/test/app/uuid.result
> index e06331001..5b9ffa230 100644
> --- a/test/app/uuid.result
> +++ b/test/app/uuid.result

2. I don't see a test for == with an incorrect string. Is it
there?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-21 15:17   ` Vladislav Shpilevoy
@ 2020-11-21 19:07     ` Oleg Babin
  2020-11-23 21:58       ` Vladislav Shpilevoy
  0 siblings, 1 reply; 21+ messages in thread
From: Oleg Babin @ 2020-11-21 19:07 UTC (permalink / raw)
  To: Vladislav Shpilevoy, lvasiliev; +Cc: tarantool-patches

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<babinoleg@mail.ru>
>>
>> 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
  ---

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings
  2020-11-21 15:17   ` Vladislav Shpilevoy
@ 2020-11-21 19:07     ` Oleg Babin
  0 siblings, 0 replies; 21+ messages in thread
From: Oleg Babin @ 2020-11-21 19:07 UTC (permalink / raw)
  To: Vladislav Shpilevoy, lvasiliev; +Cc: tarantool-patches

Hi! Thanks for your comments. See my answers below.

On 21/11/2020 18:17, Vladislav Shpilevoy wrote:
> Thanks for the patch!
>
> See 2 comments below.
>
> On 18.11.2020 08:56,olegrok@tarantool.org  wrote:
>> From: Oleg Babin<babinoleg@mail.ru>
>>
>> Before this patch it was impossible to compare uuid values with
>> string representations of uuid. However we have cases when such
>> comparisons is possible (e.g. "decimal" where we can compare
>> decimal values with strings and numbers).
>>
>> This patch extends uuid comparators (eq, lt, le) and every string
>> argument is tried to be converted to uuid value to compare then.
>>
>> Follow-up #5511
>>
>> @TarantoolBot document
>> Title: uuid values could be compared with strings
>>
>> Currently it's possible to compare uuid values with its string
>> representations:
>> ```lua
>> u1_str = 'aaaaaaaa-aaaa-4000-b000-000000000001'
>> u1 = uuid.fromstr(u1_str)
>> u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'
>>
>> u1 == u1_str -- true
>> u1 == u2_str -- false
>>
>> u1 >= u1_str -- true
>> u1 < u2_str  -- true
>> ```
> 1. Better use a single docbot request. It will be simpler for the doc
> team to handle it, since both the updates are about the same place.
> But up to you.

Agree. I've merged them.

```

     Title: uuid comparison rules

     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
     ```

     Also it's possible to compare uuid values with its string
     representations:
     ```lua
     u1_str = 'aaaaaaaa-aaaa-4000-b000-000000000001'
     u1 = uuid.fromstr(u1_str)
     u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'

     u1 == u1_str -- true
     u1 == u2_str -- false

     u1 >= u1_str -- true
     u1 < u2_str  -- true
     ```

```


>> ---
>> Issue:https://github.com/tarantool/tarantool/issues/5511
>> Branch:https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2
>>
>> diff --git a/test/app/uuid.result b/test/app/uuid.result
>> index e06331001..5b9ffa230 100644
>> --- a/test/app/uuid.result
>> +++ b/test/app/uuid.result
> 2. I don't see a test for == with an incorrect string. Is it
> there?

Yes, it was added before 
https://github.com/tarantool/tarantool/blob/e23b14dc23d749d092295a9b84854e1d64b2db27/test/app/uuid.test.lua#L89

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-21 19:07     ` Oleg Babin
@ 2020-11-23 21:58       ` Vladislav Shpilevoy
  2020-11-24  5:57         ` Oleg Babin
  2020-11-27 15:17         ` Oleg Babin
  0 siblings, 2 replies; 21+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-23 21:58 UTC (permalink / raw)
  To: Oleg Babin, lvasiliev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

> 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)

Check_uuid() here uses incorrect error index. check_uuid()
raises error(..., 4). So check_uuid() itself is level 1.
uuid_eq() is level 2. And the calling code is level 3. You
will raise one level beyond the caller code.

For uuid_lt and uuid_le it works, because check_uuid() is
level 1, uuid_cmp() is level 2, uuid_lt/le() is level 3, and
the calling code is level 4.

But I can't give you a test, since I have no idea how to provide
lhs with non-uuid type. Igor said we can give metatable of uuid to
some non-uuid value, but I failed to assign uuid metatable to
anything else.

After you fix this (but I don't insist on adding a test, because
I don't know how), I suggest you to get a second review from Igor.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-23 21:58       ` Vladislav Shpilevoy
@ 2020-11-24  5:57         ` Oleg Babin
  2020-11-24 13:06           ` Igor Munkin
  2020-11-27 15:17         ` Oleg Babin
  1 sibling, 1 reply; 21+ messages in thread
From: Oleg Babin @ 2020-11-24  5:57 UTC (permalink / raw)
  To: Vladislav Shpilevoy, Igor Munkin; +Cc: tarantool-patches

Hi! Thanks for comments!

On 24/11/2020 00:58, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
>> 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)
> Check_uuid() here uses incorrect error index. check_uuid()
> raises error(..., 4). So check_uuid() itself is level 1.
> uuid_eq() is level 2. And the calling code is level 3. You
> will raise one level beyond the caller code.
>
> For uuid_lt and uuid_le it works, because check_uuid() is
> level 1, uuid_cmp() is level 2, uuid_lt/le() is level 3, and
> the calling code is level 4.


Good catch! Thanks. Fixed in the following way:

diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
index 275f4e5c5..1171dfcc4 100644
--- a/src/lua/uuid.lua
+++ b/src/lua/uuid.lua
@@ -93,20 +93,20 @@ local uuid_isnil = function(uu)
      return builtin.tt_uuid_is_nil(uu)
  end

-local check_uuid = function(value, index)
+local check_uuid = function(value, index, err_lvl)
      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)
+    error(err_fmt:format(index), err_lvl)
  end

  local uuid_eq = function(lhs, rhs)
      if not is_uuid(rhs) then
          return false
      end
-    lhs = check_uuid(lhs, 1)
+    lhs = check_uuid(lhs, 1, 3)
      return builtin.tt_uuid_is_equal(lhs, rhs)
  end

@@ -128,8 +128,8 @@ local uuid_new_str = function()
  end

  local uuid_cmp = function(lhs, rhs)
-    lhs = check_uuid(lhs, 1)
-    rhs = check_uuid(rhs, 2)
+    lhs = check_uuid(lhs, 1, 4)
+    rhs = check_uuid(rhs, 2, 4)
      return builtin.tt_uuid_compare(lhs, rhs)
  end
  local uuid_lt = function(lhs, rhs)

> But I can't give you a test, since I have no idea how to provide
> lhs with non-uuid type. Igor said we can give metatable of uuid to
> some non-uuid value, but I failed to assign uuid metatable to
> anything else.

It's possible but I don't think it's really needed. I put small snippet 
below:

uuid = require('uuid')
eq = debug.getmetatable(uuid()).__eq
eq('', uuid()) -- error: incorrect value to compare with uuid as 1 argument
proxy = newproxy()
debug.setmetatable(proxy, {__eq = eq})
proxy == uuid() -- error: '[string "return proxy == uuid()"]:1: 
incorrect value to compare with uuid as 1 argument'

I can add it to our test suite but it looks really strange

> After you fix this (but I don't insist on adding a test, because
> I don't know how), I suggest you to get a second review from Igor.

Ok. I add Igor to CC.

Igor, could you provide the second review?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-24  5:57         ` Oleg Babin
@ 2020-11-24 13:06           ` Igor Munkin
  0 siblings, 0 replies; 21+ messages in thread
From: Igor Munkin @ 2020-11-24 13:06 UTC (permalink / raw)
  To: Oleg Babin; +Cc: tarantool-patches, Vladislav Shpilevoy

Oleg,

This is not a review, I just want to clarify Vlad's point here. I'll
make a review in a separate reply.

On 24.11.20, Oleg Babin wrote:
> Hi! Thanks for comments!
> 
> On 24/11/2020 00:58, Vladislav Shpilevoy wrote:

<snipped>

> 
> > But I can't give you a test, since I have no idea how to provide
> > lhs with non-uuid type. Igor said we can give metatable of uuid to
> > some non-uuid value, but I failed to assign uuid metatable to
> > anything else.
> 
> It's possible but I don't think it's really needed. I put small snippet 
> below:
> 
> uuid = require('uuid')
> eq = debug.getmetatable(uuid()).__eq
> eq('', uuid()) -- error: incorrect value to compare with uuid as 1 argument
> proxy = newproxy()
> debug.setmetatable(proxy, {__eq = eq})
> proxy == uuid() -- error: '[string "return proxy == uuid()"]:1: 
> incorrect value to compare with uuid as 1 argument'
> 
> I can add it to our test suite but it looks really strange

Unfortunately this simply doesn't work the way you want. Consider the
following:
| $ ./src/tarantool
| Tarantool 2.7.0-42-g374917337
| type 'help' for interactive help
| tarantool> require('ffi').typeof(box.error.new(box.error.UNKNOWN))
| ---
| - ctype<const struct error &>
| ...
| 
| tarantool> require('ffi').typeof(require('uuid')())
| ---
| - ctype<struct tt_uuid>
| ...
| 
| tarantool> debug.getmetatable(box.error.new(box.error.UNKNOWN)) == debug.getmetatable(require('uuid')())
| ---
| - true
| ...
| 
| tarantool>

Oops... It occurs ctype<const struct error &> shares the same metatable
as ctype<struct tt_uuid>, doesn't it? Yes, it does. At this place FFI
conforms Lua reference manual section[1] describing the metatables:
| Tables and full userdata have individual metatables (although multiple
| tables and userdata can share their metatables). Values of all other
| types share one single metatable per type; that is, there is one single
| metatable for all numbers, one for all strings, etc.

I guess there is no legal (i.e. using only Lua with no LuaJIT internals)
way to check that the function you obtained in the test is
<lj_cf_ffi_meta___eq> fast function (although, you can check yourself
via luajit-gdb.py), so just trust me. */me making Jedi mind tricks here*

Anyway, how does this magic work? We need to distinguish *metatables*
and *metatypes* -- the latter one is specific only for GCcdata. The
metatype metamethod is chosen underneath <lj_cf_ffi_meta___eq>
considering the ctype passed via the one of the arguments. I've already
described metatype metamethods resolution in the previous reply.

There is uuid in both of the tests above, so we can see only the error
related to the miscomparison, but AFAIU Vlad asked about the error
raised on check_uuid (with the "Usage: blah-blah" error message). The
current implementation provides no way to reproduce it (maybe via other
debug.* interfaces, I haven't checked this yet).

> 

<snipped>

> 

[1]: https://www.lua.org/manual/5.1/manual.html#2.8

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua olegrok
  2020-11-21 15:17   ` Vladislav Shpilevoy
@ 2020-11-24 15:20   ` Igor Munkin
  2020-11-24 19:23     ` Oleg Babin
  1 sibling, 1 reply; 21+ messages in thread
From: Igor Munkin @ 2020-11-24 15:20 UTC (permalink / raw)
  To: olegrok; +Cc: tarantool-patches, v.shpilevoy

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 <babinoleg@mail.ru>
> 
> 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.

> 
> 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(+)
> 

<snipped>

> 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

<snipped>

> +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.

> +

<snipped>

> -- 
> 2.29.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings
  2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings olegrok
  2020-11-21 15:17   ` Vladislav Shpilevoy
@ 2020-11-24 15:20   ` Igor Munkin
  2020-11-24 19:23     ` Oleg Babin
  1 sibling, 1 reply; 21+ messages in thread
From: Igor Munkin @ 2020-11-24 15:20 UTC (permalink / raw)
  To: olegrok; +Cc: tarantool-patches, v.shpilevoy

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 <babinoleg@mail.ru>
> 
> Before this patch it was impossible to compare uuid values with
> string representations of uuid. However we have cases when such
> comparisons is possible (e.g. "decimal" where we can compare
> decimal values with strings and numbers).
> 
> This patch extends uuid comparators (eq, lt, le) and every string

Minor: Strictly saying, you're right, but AFAICS we usually use the key
name (i.e. __eq, __le and __lt) for metamethods in description.

> argument is tried to be converted to uuid value to compare then.
> 
> Follow-up #5511
> 
> @TarantoolBot document
> Title: uuid values could be compared with strings
> 
> Currently it's possible to compare uuid values with its string
> representations:
> ```lua
> u1_str = 'aaaaaaaa-aaaa-4000-b000-000000000001'
> u1 = uuid.fromstr(u1_str)
> u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'
> 
> u1 == u1_str -- true
> u1 == u2_str -- false
> 
> u1 >= u1_str -- true
> u1 < u2_str  -- true
> ```
> ---
> Issue: https://github.com/tarantool/tarantool/issues/5511
> Branch: https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2
> 
>  src/lua/uuid.lua       | 31 +++++++++++++--
>  test/app/uuid.result   | 85 ++++++++++++++++++++++++++++++++++++++----
>  test/app/uuid.test.lua | 27 ++++++++++++++
>  3 files changed, 131 insertions(+), 12 deletions(-)
> 

<snipped>

> -- 
> 2.29.0
> 

-- 
Best regards,
IM

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-24 15:20   ` Igor Munkin
@ 2020-11-24 19:23     ` Oleg Babin
  0 siblings, 0 replies; 21+ messages in thread
From: Oleg Babin @ 2020-11-24 19:23 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches, v.shpilevoy

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<babinoleg@mail.ru>
>>
>> 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(+)
>>
> <snipped>
>
>> 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
> <snipped>
>
>> +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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings
  2020-11-24 15:20   ` Igor Munkin
@ 2020-11-24 19:23     ` Oleg Babin
  0 siblings, 0 replies; 21+ messages in thread
From: Oleg Babin @ 2020-11-24 19:23 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches, v.shpilevoy

Thanks for 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<babinoleg@mail.ru>
>>
>> Before this patch it was impossible to compare uuid values with
>> string representations of uuid. However we have cases when such
>> comparisons is possible (e.g. "decimal" where we can compare
>> decimal values with strings and numbers).
>>
>> This patch extends uuid comparators (eq, lt, le) and every string
> Minor: Strictly saying, you're right, but AFAICS we usually use the key
> name (i.e. __eq, __le and __lt) for metamethods in description.

I've updated and force-pushed branch. I cite only changed part:

     This patch extends uuid comparators (__eq, __lt and __le) and
     every string argument is tried to be converted to uuid value to
     compare then.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua
  2020-11-23 21:58       ` Vladislav Shpilevoy
  2020-11-24  5:57         ` Oleg Babin
@ 2020-11-27 15:17         ` Oleg Babin
  1 sibling, 0 replies; 21+ messages in thread
From: Oleg Babin @ 2020-11-27 15:17 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi!

On 24/11/2020 00:58, Vladislav Shpilevoy wrote:
> After you fix this (but I don't insist on adding a test, because
> I don't know how), I suggest you to get a second review from Igor.


Vlad, I've got LGTM for the patchset from Igor. Could you please 
continue your review?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable
  2020-11-18  7:56 [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable olegrok
                   ` (2 preceding siblings ...)
  2020-11-18  8:02 ` [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable Oleg Babin
@ 2020-11-27 22:39 ` Vladislav Shpilevoy
  2020-11-27 22:40 ` Vladislav Shpilevoy
  2020-12-07 23:24 ` Vladislav Shpilevoy
  5 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-27 22:39 UTC (permalink / raw)
  To: olegrok, lvasiliev; +Cc: tarantool-patches

Hi! Thanks for the patch!

LGTM.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable
  2020-11-18  7:56 [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable olegrok
                   ` (3 preceding siblings ...)
  2020-11-27 22:39 ` Vladislav Shpilevoy
@ 2020-11-27 22:40 ` Vladislav Shpilevoy
  2020-12-04  8:13   ` Oleg Babin
  2020-12-07 23:24 ` Vladislav Shpilevoy
  5 siblings, 1 reply; 21+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-27 22:40 UTC (permalink / raw)
  To: olegrok, lvasiliev, Alexander V. Tikhonov; +Cc: tarantool-patches

Sasha (Tikh.), please, take a look at CI and tell if we can push this.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable
  2020-11-27 22:40 ` Vladislav Shpilevoy
@ 2020-12-04  8:13   ` Oleg Babin
  2020-12-07 23:04     ` Alexander V. Tikhonov
  0 siblings, 1 reply; 21+ messages in thread
From: Oleg Babin @ 2020-12-04  8:13 UTC (permalink / raw)
  To: Vladislav Shpilevoy, Alexander V. Tikhonov; +Cc: tarantool-patches

Sasha, CI [1] on my branch [2] is green. Could you confirm we can push 
this patch?


[1] https://gitlab.com/tarantool/tarantool/-/pipelines/220727298

[2] https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2

On 28/11/2020 01:40, Vladislav Shpilevoy wrote:
> Sasha (Tikh.), please, take a look at CI and tell if we can push this.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable
  2020-12-04  8:13   ` Oleg Babin
@ 2020-12-07 23:04     ` Alexander V. Tikhonov
  0 siblings, 0 replies; 21+ messages in thread
From: Alexander V. Tikhonov @ 2020-12-07 23:04 UTC (permalink / raw)
  To: Oleg Babin; +Cc: tarantool-patches

Hi Oleg, , thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline, patch LGTM.

On Fri, Dec 04, 2020 at 11:13:09AM +0300, Oleg Babin wrote:
> Sasha, CI [1] on my branch [2] is green. Could you confirm we can push this
> patch?
> 
> 
> [1] https://gitlab.com/tarantool/tarantool/-/pipelines/220727298
> 
> [2] https://github.com/tarantool/tarantool/tree/olegrok/5511-uuid-cmp-v2
> 
> On 28/11/2020 01:40, Vladislav Shpilevoy wrote:
> > Sasha (Tikh.), please, take a look at CI and tell if we can push this.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable
  2020-11-18  7:56 [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable olegrok
                   ` (4 preceding siblings ...)
  2020-11-27 22:40 ` Vladislav Shpilevoy
@ 2020-12-07 23:24 ` Vladislav Shpilevoy
  5 siblings, 0 replies; 21+ messages in thread
From: Vladislav Shpilevoy @ 2020-12-07 23:24 UTC (permalink / raw)
  To: olegrok, lvasiliev; +Cc: tarantool-patches

Pushed to master.

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-12-07 23:24 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-18  7:56 [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable olegrok
2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua olegrok
2020-11-21 15:17   ` Vladislav Shpilevoy
2020-11-21 19:07     ` Oleg Babin
2020-11-23 21:58       ` Vladislav Shpilevoy
2020-11-24  5:57         ` Oleg Babin
2020-11-24 13:06           ` Igor Munkin
2020-11-27 15:17         ` Oleg Babin
2020-11-24 15:20   ` Igor Munkin
2020-11-24 19:23     ` Oleg Babin
2020-11-18  7:56 ` [Tarantool-patches] [PATCH v2 2/2] uuid: support uuid comparison with strings olegrok
2020-11-21 15:17   ` Vladislav Shpilevoy
2020-11-21 19:07     ` Oleg Babin
2020-11-24 15:20   ` Igor Munkin
2020-11-24 19:23     ` Oleg Babin
2020-11-18  8:02 ` [Tarantool-patches] [PATCH v2 0/2] Make uuid values comparable Oleg Babin
2020-11-27 22:39 ` Vladislav Shpilevoy
2020-11-27 22:40 ` Vladislav Shpilevoy
2020-12-04  8:13   ` Oleg Babin
2020-12-07 23:04     ` Alexander V. Tikhonov
2020-12-07 23:24 ` Vladislav Shpilevoy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox