Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.