From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149])
 (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 4B1F24696C4
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 10 Apr 2020 19:56:44 +0300 (MSK)
References: <cover.1586476073.git.sergepetrenko@tarantool.org>
 <d9b105fae73e52d7dc45fe2c3cf0725eef1d3355.1586476073.git.sergepetrenko@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <2df5574a-10b4-c086-ce76-6d8b5df3aef4@tarantool.org>
Date: Fri, 10 Apr 2020 18:56:43 +0200
MIME-Version: 1.0
In-Reply-To: <d9b105fae73e52d7dc45fe2c3cf0725eef1d3355.1586476073.git.sergepetrenko@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v2 4/4] box: introduce indices by
	UUID
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Serge Petrenko <sergepetrenko@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

Thanks for the patch!

The patchset is almost perfect, just a few nits are left.
See 2 of them below.

> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index 3f8a0ce24..a47f7ac6d 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -378,6 +382,25 @@ mp_compare_bin(const char *field_a, const char *field_b)
>  	return COMPARE_RESULT(size_a, size_b);
>  }
>  
> +static inline int
> +mp_compare_uuid(const char *field_a, const char *field_b)
> +{
> +	const char *str_a, *str_b;
> +	int8_t type;
> +	uint32_t len;
> +	str_a = mp_decode_ext(&field_a, &type, &len);
> +	assert(type == MP_UUID && len == UUID_PACKED_LEN);
> +	str_b = mp_decode_ext(&field_b, &type, &len);
> +	assert(type == MP_UUID && len == UUID_PACKED_LEN);

1. I would either do field_a += 2, field_b += 2; or just
memcmp(field_a, field_b), because the same prefix won't
affect the result. Up to you. +2 is the fastest solution I
think.

+2 version could be even moved to mp_uuid.h as something like
mp_strip_uuid_header() or mp_decode_ext_uuidl() (similar to
mp_decode_strl()). Point is we eliminate lots of instructions
and switch-case mp_decode_extl().

> +	/*
> +	 * Packed uuid fields are in the right order for
> +	 * comparison and are big-endian, so memcmp is
> +	 * the same as tt_uuid_compare() and lets us
> +	 * spare 2 mp_uuid_unpack() calls.
> +	 */
> +	return memcmp(str_a, str_b, UUID_PACKED_LEN);
> +}> diff --git a/test/engine/uuid.result b/test/engine/uuid.result
> new file mode 100644
> index 000000000..c4c186e92
> --- /dev/null
> +++ b/test/engine/uuid.result
> @@ -0,0 +1,55 @@
> +-- test-run result file version 2
> +env = require('test_run')
> + | ---
> + | ...
> +test_run = env.new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
> + | ...
> +
> +uuid = require('uuid')
> + | ---
> + | ...
> +ffi = require('ffi')
> + | ---
> + | ...
> +
> +-- check uuid indices

2. Lets mention the ticket here, and

    check -> Check
    indices -> indices.

> +_ = box.schema.space.create('test', {engine=engine})
> + | ---
> + | ...
> +_ = box.space.test:create_index('pk', {parts={1,'uuid'}})
> + | ---
> + | ...