Re: [Tarantool-patches] [PATCH v2 4/4] box: introduce indices by UUID

[Serge Petrenko <sergepetrenko@tarantool.org>]

> 10 апр. 2020 г., в 19:56, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Thanks for the patch!
> 

Hi! Thanks for the review!

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

Lets just do field_a + 2, field_b + 2.

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

Ok.

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


diff --git a/src/box/field_def.c b/src/box/field_def.c
index 82a2493fa..213e91699 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -33,7 +33,7 @@
 #include "trivia/util.h"
 #include "key_def.h"
 #include "mp_extension_types.h"
-#include "lib/uuid/mp_uuid.h"
+#include "uuid/mp_uuid.h"
 #include "uuid/tt_uuid.h"
 
 const char *mp_type_strs[] = {
diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
index a47f7ac6d..bc01fe068 100644
--- a/src/box/tuple_compare.cc
+++ b/src/box/tuple_compare.cc
@@ -35,7 +35,7 @@
 #include <math.h>
 #include "lib/core/decimal.h"
 #include "lib/core/mp_decimal.h"
-#include "lib/uuid/mp_uuid.h"
+#include "uuid/mp_uuid.h"
 #include "lib/core/mp_extension_types.h"
 
 /* {{{ tuple_compare */
@@ -385,20 +385,14 @@ mp_compare_bin(const char *field_a, const char *field_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);
 	/*
 	 * 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.
+	 * "field_a + 2" to skip the uuid header.
 	 */
-	return memcmp(str_a, str_b, UUID_PACKED_LEN);
+	return memcmp(field_a + 2, field_b + 2, UUID_PACKED_LEN);
 }
 
 typedef int (*mp_compare_f)(const char *, const char *);
diff --git a/test/engine/uuid.test.lua b/test/engine/uuid.test.lua
index f34795ce4..34e74e68b 100644
--- a/test/engine/uuid.test.lua
+++ b/test/engine/uuid.test.lua
@@ -5,7 +5,7 @@ engine = test_run:get_cfg('engine')
 uuid = require('uuid')
 ffi = require('ffi')
 
--- check uuid indices
+-- Check uuid indices (gh-4268).
 _ = box.schema.space.create('test', {engine=engine})
 _ = box.space.test:create_index('pk', {parts={1,'uuid'}})
 


--
Serge Petrenko
sergepetrenko@tarantool.org