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

[Oleg Babin <olegrok@tarantool.org>]

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.