[Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[olegrok]

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.
To be consistent with decimal type this patch allows to compare
uuid with uuid string representation.

@TarantoolBot document Title: uuid compare

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

-- Comparison with string uuid representation supported as well

u1 > 'bbbbbbbb-bbbb-4000-b000-000000000001' -- false
u1 < 'bbbbbbbb-bbbb-4000-b000-000000000001' -- true
u1 < 'not-uuid-string' -- error

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

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

 src/exports.h          |  1 +
 src/lua/uuid.lua       | 31 +++++++++++++
 test/app/uuid.result   | 98 ++++++++++++++++++++++++++++++++++++++++++
 test/app/uuid.test.lua | 35 +++++++++++++++
 4 files changed, 165 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..0ab937877 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,38 @@ local uuid_new_str = function()
     return uuid_tostring(uuidbuf)
 end
 
+local check_uuid = function(value, index)
+    if is_uuid(value) then
+        return value
+    end
+
+    if type(value) == 'string' then
+        value = uuid_fromstr(value)
+        if value ~= nil then
+            return value
+        end
+    end
+
+    error(('incorrect value to convert to uuid as %d argument'):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..36388aed3 100644
--- a/test/app/uuid.result
+++ b/test/app/uuid.result
@@ -291,6 +291,104 @@ uuid.is_uuid(require('decimal').new('123'))
 ---
 - false
 ...
+--
+-- gh-5511: allow to compare uuid values
+--
+u1 = uuid.fromstr('aaaaaaaa-aaaa-4000-b000-000000000001')
+---
+...
+u2_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'
+---
+...
+u2 = uuid.fromstr(u2_str)
+---
+...
+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 > u2_str
+---
+- false
+...
+u1 >= u2_str
+---
+- false
+...
+u1 <= u2_str
+---
+- true
+...
+u1 < u2_str
+---
+- true
+...
+u1 < 1
+---
+- error: incorrect value to convert to uuid as 2 argument
+...
+u1 <= 1
+---
+- error: incorrect value to convert to uuid as 2 argument
+...
+u1 < 'abc'
+---
+- error: incorrect value to convert to uuid as 2 argument
+...
+u1 <= 'abc'
+---
+- error: incorrect value to convert to uuid as 2 argument
+...
+1 < u1
+---
+- error: incorrect value to convert to uuid as 1 argument
+...
+1 <= u1
+---
+- error: incorrect value to convert to uuid as 1 argument
+...
+'abc' < u1
+---
+- error: incorrect value to convert to uuid as 1 argument
+...
+'abc' <= u1
+---
+- error: incorrect value to convert to 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..5a57976e1 100644
--- a/test/app/uuid.test.lua
+++ b/test/app/uuid.test.lua
@@ -108,6 +108,41 @@ 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_str = 'bbbbbbbb-bbbb-4000-b000-000000000001'
+u2 = uuid.fromstr(u2_str)
+
+u1 > u1
+u1 >= u1
+u1 <= u1
+u1 < u1
+
+u1 > u2
+u1 >= u2
+u1 <= u2
+u1 < u2
+
+u1 > u2_str
+u1 >= u2_str
+u1 <= u2_str
+u1 < u2_str
+
+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

Re: [Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Sergey Bronnikov]

Hi, Oleg

thanks for the patch!
Looks like you expect a review from Leonid and Vlad, but I have a
question regarding tests in the patch.

On 23:04 Wed 11 Nov , olegrok@tarantool.org wrote:
> --- a/test/app/uuid.result
> +++ b/test/app/uuid.result

For Tarantool core we have two kind of tests: with diff-based approach
and TAP based tests. Your tests can be easily implemented with a TAP
approach, when test data and oracle are in the code. But you preferred a
diff-based approach here. Why?

Re: [Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Oleg Babin]

Hi, Sergey!

The main reason is "uuid tests were implemented with diff-based approach".

However I can easily convert my tests to TAP but I don't want touch 
existing tests of course.

Do you think we should start moving towards TAP-based approach for 
"uuid" module testing?


On 12/11/2020 12:44, Sergey Bronnikov wrote:
> Hi, Oleg
>
> thanks for the patch!
> Looks like you expect a review from Leonid and Vlad, but I have a
> question regarding tests in the patch.
>
> On 23:04 Wed 11 Nov , olegrok@tarantool.org wrote:
>> --- a/test/app/uuid.result
>> +++ b/test/app/uuid.result
> For Tarantool core we have two kind of tests: with diff-based approach
> and TAP based tests. Your tests can be easily implemented with a TAP
> approach, when test data and oracle are in the code. But you preferred a
> diff-based approach here. Why?

Re: [Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Sergey Bronnikov]

Hi, Oleg

On 12.11.2020 12:55, Oleg Babin wrote:
> Hi, Sergey!
>
> The main reason is "uuid tests were implemented with diff-based approach".

Got it. Missed it that you haven't add new test and just enhanced the 
old one.

>
> However I can easily convert my tests to TAP but I don't want touch 
> existing tests of course.
>
> Do you think we should start moving towards TAP-based approach for 
> "uuid" module testing?

I think no.

> On 12/11/2020 12:44, Sergey Bronnikov wrote:
>> Hi, Oleg
>>
>> thanks for the patch!
>> Looks like you expect a review from Leonid and Vlad, but I have a
>> question regarding tests in the patch.
>>
>> On 23:04 Wed 11 Nov , olegrok@tarantool.org wrote:
>>> --- a/test/app/uuid.result
>>> +++ b/test/app/uuid.result
>> For Tarantool core we have two kind of tests: with diff-based approach
>> and TAP based tests. Your tests can be easily implemented with a TAP
>> approach, when test data and oracle are in the code. But you preferred a
>> diff-based approach here. Why?

Re: [Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

Lets change subsystem from 'lua:' to 'uuid:'. The former is too
general.

See 4 comments below.

On 11.11.2020 21:04, 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.
> To be consistent with decimal type this patch allows to compare
> uuid with uuid string representation.
> 
> @TarantoolBot document Title: uuid compare

1. I am afraid it won't work this way. You need to have 'Title' on
separate line. You can see if your comment works here:
https://tarantool-docbot.herokuapp.com, right after you pushed the
commit.

> 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..0ab937877 100644
> --- a/src/lua/uuid.lua
> +++ b/src/lua/uuid.lua
> @@ -118,9 +120,38 @@ local uuid_new_str = function()
>      return uuid_tostring(uuidbuf)
>  end
>  
> +local check_uuid = function(value, index)
> +    if is_uuid(value) then
> +        return value
> +    end
> +
> +    if type(value) == 'string' then
> +        value = uuid_fromstr(value)

2. This may be expensive to do for each comparison with a string.
If you care, try to use static_alloc so as not to allocate the
temporary tt_uuid on the heap. It is already used in this file,
you can check out some usage examples.

> +        if value ~= nil then
> +            return value
> +        end
> +    end
> +
> +    error(('incorrect value to convert to uuid as %d argument'):format(index), 0)

3. Out of 80 symbols.

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

4. Hmmmmm

tarantool> u1 = 'b0b681e9-dd43-4341-98b4-a973b23f1421'
---
...

tarantool> u2 = uuid()
---
...

tarantool> type(u2)
---
- cdata
...

tarantool> type(u1)
---
- string
...

tarantool> u2 < u1
---
- true
...

tarantool> u1 > u2
---
- true
...

Doesn't this look wrong?

From what I see here: http://lua-users.org/wiki/MetamethodsTutorial
they say, that for '<' Lua uses metatable of the left object. It means,
that if string is on the left, it will use string metamethods. Although
I may be wrong, and you need to check how it actually works. If it is
true, I am afraid we can't support string vs cdata-uuid comparison.
Only cdata-uuid vs cdata-uuid. For string case you may need to add a new
function. Probably in your own application.

Re: [Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Igor Munkin]

Vlad,

I glanced the patch and your review and want to clarify one point.

On 16.11.20, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> Lets change subsystem from 'lua:' to 'uuid:'. The former is too
> general.
> 
> See 4 comments below.
> 
> On 11.11.2020 21:04, 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.
> > To be consistent with decimal type this patch allows to compare
> > uuid with uuid string representation.
> > 
> > @TarantoolBot document Title: uuid compare
> 

<snipped>

> 
> 4. Hmmmmm
> 
> tarantool> u1 = 'b0b681e9-dd43-4341-98b4-a973b23f1421'
> ---
> ...
> 
> tarantool> u2 = uuid()
> ---
> ...
> 
> tarantool> type(u2)
> ---
> - cdata
> ...
> 
> tarantool> type(u1)
> ---
> - string
> ...
> 
> tarantool> u2 < u1
> ---
> - true
> ...
> 
> tarantool> u1 > u2
> ---
> - true
> ...
> 
> Doesn't this look wrong?

Yes and no. At first result is the same. The issue you mentioned above
relates to cdata comparison, but this is wrong in general. According to
Lua Reference manual[1] the objects can be compared via the metamethods
iff they are the same types. Otherwise the comparison fails. Consider
the following example:
| $ tarantool
| Tarantool 1.10.8-0-g2f18757
| type 'help' for interactive help
| tarantool> a = setmetatable({ a = 'QQ' }, { __lt = function(self, obj) if type(obj) ~= 'string' then error('invalid') end return self.a < obj end })
| ---
| ...
| 
| tarantool> str = 'QKRQ'
| ---
| ...
| 
| tarantool> a < str
| ---
| - error: '[string "return a < str"]:1: attempt to compare table with string'
| ...

Anyway, cdata comparison is badly described on LuaJIT FFI semantics
page[2], but at least there is the following note:
| Comparisons for equality/inequality never raise an error. Even
| incompatible pointers can be compared for equality by address. Any
| other incompatible comparison (also with non-cdata objects) treats the
| two sides as unequal.

At the same time, Mike implemented kinda __rlt for cdata types. In other
words, if the left operand is not cdata, but the right one is, then it
would lookup the right one for __lt. I failed to find the place this
freaking hack is described but this is how it works.

> 
> From what I see here: http://lua-users.org/wiki/MetamethodsTutorial
> they say, that for '<' Lua uses metatable of the left object. It means,
> that if string is on the left, it will use string metamethods. Although
> I may be wrong, and you need to check how it actually works. If it is
> true, I am afraid we can't support string vs cdata-uuid comparison.

Looks like we can, *but* I only checked your example, so I see no other
pitfalls for now. Anyway, I guess I will make a precise description for
the way cdata metamethod are looked up.

> Only cdata-uuid vs cdata-uuid. For string case you may need to add a new
> function. Probably in your own application.

[1]: https://www.lua.org/manual/5.1/manual.html#2.8
[2]: http://luajit.org/ext_ffi_semantics.html

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Vladislav Shpilevoy]

Hi! Thanks for the reply!

>> tarantool> u1 = 'b0b681e9-dd43-4341-98b4-a973b23f1421'
>> ---
>> ...
>>
>> tarantool> u2 = uuid()
>> ---
>> ...
>>
>> tarantool> type(u2)
>> ---
>> - cdata
>> ...
>>
>> tarantool> type(u1)
>> ---
>> - string
>> ...
>>
>> tarantool> u2 < u1
>> ---
>> - true
>> ...
>>
>> tarantool> u1 > u2
>> ---
>> - true
>> ...
>>
>> Doesn't this look wrong?
> 
> Yes and no. At first result is the same.

Oh, shit. I am so stupid. Sorry, Oleg and Igor, my bad.

My comment about comparison being wrong is not fair, obviously.

But still propose to use static_alloc. I do not insist. Only if perf
matters. (For me it matters always, so I would try.)

Re: [Tarantool-patches] [PATCH] lua: introduce function for uuid comparison

[Oleg Babin]

Hi! Thanks for your comments. I send new version of this patch. See my 
answers below.

On 17/11/2020 00:36, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
>
> Lets change subsystem from 'lua:' to 'uuid:'. The former is too
> general.
>
> See 4 comments below.
>
> On 11.11.2020 21:04, 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.
>> To be consistent with decimal type this patch allows to compare
>> uuid with uuid string representation.
>>
>> @TarantoolBot document Title: uuid compare
> 1. I am afraid it won't work this way. You need to have 'Title' on
> separate line. You can see if your comment works here:
> https://tarantool-docbot.herokuapp.com, right after you pushed the
> commit.
>
Fixed. Thanks!


>> 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..0ab937877 100644
>> --- a/src/lua/uuid.lua
>> +++ b/src/lua/uuid.lua
>> @@ -118,9 +120,38 @@ local uuid_new_str = function()
>>       return uuid_tostring(uuidbuf)
>>   end
>>   
>> +local check_uuid = function(value, index)
>> +    if is_uuid(value) then
>> +        return value
>> +    end
>> +
>> +    if type(value) == 'string' then
>> +        value = uuid_fromstr(value)
> 2. This may be expensive to do for each comparison with a string.
> If you care, try to use static_alloc so as not to allocate the
> temporary tt_uuid on the heap. It is already used in this file,
> you can check out some usage examples.


Thanks for advice. Fixed.

>> +        if value ~= nil then
>> +            return value
>> +        end
>> +    end
>> +
>> +    error(('incorrect value to convert to uuid as %d argument'):format(index), 0)
> 3. Out of 80 symbols.

Fixed


>> +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
> 4. Hmmmmm
>
> tarantool> u1 = 'b0b681e9-dd43-4341-98b4-a973b23f1421'
> ---
> ...
>
> tarantool> u2 = uuid()
> ---
> ...
>
> tarantool> type(u2)
> ---
> - cdata
> ...
>
> tarantool> type(u1)
> ---
> - string
> ...
>
> tarantool> u2 < u1
> ---
> - true
> ...
>
> tarantool> u1 > u2
> ---
> - true
> ...
>
> Doesn't this look wrong?
>
>  From what I see here: http://lua-users.org/wiki/MetamethodsTutorial
> they say, that for '<' Lua uses metatable of the left object. It means,
> that if string is on the left, it will use string metamethods. Although
> I may be wrong, and you need to check how it actually works. If it is
> true, I am afraid we can't support string vs cdata-uuid comparison.
> Only cdata-uuid vs cdata-uuid. For string case you may need to add a new
> function. Probably in your own application.

I'm grateful to Igor it described how it works.

Initially this changed was inspired by decimal module. Where we can to 
compare values with strings and numbers.

```

tarantool> require('decimal').new(1111.111) == '1111.111'
---
- true
...

tarantool> require('decimal').new(1111.111) == 1111.111
---
- true
...

```

Unfortunately, I've missed to implement such behaviour for "eq" 
function. So, I've done it in the second version.