From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 526686C1AE; Sat, 22 May 2021 19:04:19 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 526686C1AE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1621699459; bh=QmUBlVXQbn1MqGwSlk1yD/0i2oKo9QUxhR2fKEGoHdI=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=k9SjVZlbC/NYe/QiOGk6CGeiF911DCMTLay6lE2l2ll6Eu+woKdgD+Qli/SDSbpky 5DxRwfy+ePqLrvUNxdCbRsRF0F4iMHPnH0R0xKt5df03MFEA6GcAXy/vnto71SOKxR cnAkP52VRI4LvzjNWr7sgRscLCxiOMPBUCSkdpm4= Received: from smtp54.i.mail.ru (smtp54.i.mail.ru [217.69.128.34]) (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 481506C1AE for ; Sat, 22 May 2021 19:04:18 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 481506C1AE Received: by smtp54.i.mail.ru with esmtpa (envelope-from ) id 1lkU6r-0006BC-8L; Sat, 22 May 2021 19:04:17 +0300 To: imeevma@tarantool.org Cc: tarantool-patches@dev.tarantool.org References: <9d53f29d460d7a4194cb3f95de4b5a4016453db3.1621503852.git.imeevma@gmail.com> Message-ID: <2cf25c21-9522-446d-3cf9-d36e0523d24b@tarantool.org> Date: Sat, 22 May 2021 18:04:15 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.2 MIME-Version: 1.0 In-Reply-To: <9d53f29d460d7a4194cb3f95de4b5a4016453db3.1621503852.git.imeevma@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD91B019B01C53E51AFCAACD197781D6F0CB42DE8FB7A42148700894C459B0CD1B9687D83812B2624C0F02B2AB7A91749E2F7D523755E2FECAF21F44083463E5A52 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79A2E61952DECAF71EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372CFCDE0CF3C283B78638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B28120E5B6827BE9C754ED653435E03D643EADAC849D614696D2E47CDBA5A96583C09775C1D3CA48CF17B107DEF921CE79117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7328B01A8D746D8839FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE7744CB235B443BC28D32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C0967168D0166C962CCD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5AD3811C5121562704ED8E84F6E9AA9FBE37D81CA451019E8D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA752546FE575EB473F1410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3480A9008907CB2FD023F70A972DB6D145246ABFCD215DE1B0E4B1479B325FAD1956E2D4A427DC80091D7E09C32AA3244CCECF46618A2EB5501D212AD8D9F8827124AF4FAF06DA24FD729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj3CIvDNz8QqDXsYHd8CmeUg== X-Mailru-Sender: 504CC1E875BF3E7D9BC0E5172ADA3110655253E051162BBC0DCDDB25EB3E846E4600E1F4420D7A3E07784C02288277CA03E0582D3806FB6A5317862B1921BA260ED6CFD6382C13A6112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 1/1] sql: introduce UUID field type X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi! Thanks for the patch! See 15 comments below. > sql: introduce UUID field type > > This patch introduces UUID to SQL. UUID is now available as a new field > type. > > Closes #5886 > > @TarantoolBot document > Title: Field type UUID is now available in SQL > > The UUID field type is now available in SQL. This means that we can > create spaces and indexes with UUID, use it in SELECT, UPDATE and > DELETE. UUID can be accepted and returned by built-in functions and > user-defined functions. > > During comparison, UUID cannot be implicitly converted to any other > types, but in any other case, UUID can be implicitly converted to STRING > or VARBINARY. 1. I couldn't parse this - can it be implicitly converted to STRING/VARBINARY or not? What is this "any other case"? In the RFC I see that UUID should be ok to be implicitly converted to STRING and VARBINARY. > UUID can be explicitly converted to STRING or VARBINARY > using CAST(). STRING and VARBINARY can be implicitly converted to UUID, > except for comparison. STRING and VARBINARY can be explicitly converted > to UUID using CAST(). > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > index 47845599e..9288ef341 100644 > --- a/src/box/sql/mem.c > +++ b/src/box/sql/mem.c > @@ -58,10 +58,22 @@ enum { > BUF_SIZE = 32, > }; > > +bool > +mem_is_field_compatible(const struct Mem *mem, enum field_type type, > + bool is_nullable) 2. Nullable flag is passed as true in the only usage place of this function. I propose to inline it here to true and drop the argument. > +{ > + if (mem->type == MEM_TYPE_UUID) > + return (field_ext_type[type] & (1U << MP_UUID)) != 0; > + enum mp_type mp_type = mem_mp_type(mem); > + assert(mp_type != MP_EXT); > + return field_mp_plain_type_is_compatible(type, mp_type, is_nullable); > +} > @@ -81,6 +93,9 @@ mem_str(const struct Mem *mem) > case MEM_TYPE_MAP: > case MEM_TYPE_ARRAY: > return mp_str(mem->z); > + case MEM_TYPE_UUID: > + tt_uuid_to_string(&mem->u.uuid, &buf[0]); > + return tt_sprintf("%s", buf); 3. You don't need to change buf size. Use tt_uuid_str(). > case MEM_TYPE_BOOL: > return mem->u.b ? "TRUE" : "FALSE"; > default: > @@ -190,6 +205,16 @@ mem_set_double(struct Mem *mem, double value) > mem->type = MEM_TYPE_DOUBLE; > } > > +void > +mem_set_uuid(struct Mem *mem, struct tt_uuid *uuid) 4. You can make uuid argument const. > +{ > + mem_clear(mem); > + mem->field_type = FIELD_TYPE_UUID; > + mem->u.uuid = *uuid; > + mem->type = MEM_TYPE_UUID; > + mem->flags = 0; 5. Flags should be already 0 after clear(). > +} > + > static inline void > set_str_const(struct Mem *mem, char *value, uint32_t len, int alloc_type) > { > @@ -585,6 +610,18 @@ str_to_bin(struct Mem *mem) > return 0; > } > > +static inline int > +str_to_uuid(struct Mem *mem) > +{ > + assert(mem->type == MEM_TYPE_STR); > + if (tt_uuid_from_string(tt_cstr(mem->z, mem->n), &mem->u.uuid) != 0) > + return -1; 6. This is going to crash if mem->n is more than the static buffer size. I would suggest to introduce some way to extract UUID from a non-terminated string, and add a test maybe. The same in mem_get_uuid(). Which is not used anywhere btw. Perhaps you can drop it. > @@ -639,6 +676,20 @@ bin_to_str0(struct Mem *mem) > return 0; > } > > +static inline int > +bin_to_uuid(struct Mem *mem) > +{ > + assert(mem->type == MEM_TYPE_BIN); > + if (mem->n != UUID_LEN || > + tt_uuid_validate((struct tt_uuid *)mem->z) != 0) 7. What if this is MEM_Zero and z is not allocated yet? > + return -1; > + mem->u.uuid = *(struct tt_uuid *)mem->z; > + mem->type = MEM_TYPE_UUID; > + mem->flags = 0; > + mem->field_type = FIELD_TYPE_UUID; > + return 0; > +} > @@ -2566,11 +2710,24 @@ sqlVdbeCompareMsgpack(const char **key1, > case MP_ARRAY: > case MP_MAP: > case MP_EXT:{ > - mem1.z = (char *)aKey1; > - mp_next(&aKey1); > - mem1.n = aKey1 - (char *)mem1.z; > - goto do_blob; > + int8_t type; > + const char *buf = aKey1; > + uint32_t len = mp_decode_extl(&buf, &type); 8. This is going to crash when type is MP_ARRAY or MP_MAP. Is it possible to add a test? > + buf = aKey1; > + mp_next(&aKey1); 9. You can skip faster, using buf + len which you already decoded. The same in mem_from_mp_ephemeral(). > @@ -2849,6 +3023,11 @@ port_vdbemem_dump_lua(struct port *base, struct lua_State *L, bool is_flat) > case MEM_TYPE_BOOL: > lua_pushboolean(L, mem->u.b); > break; > + case MEM_TYPE_UUID: { > + struct tt_uuid *uuid = luaL_pushuuid(L); > + *uuid = mem->u.uuid; 10. You could do *luaL_pushuuid(L) = mem->u.uuid right away, without the temporary variable. Up to you. > @@ -3073,7 +3249,18 @@ port_c_get_vdbemem(struct port *base, uint32_t *size) > break; > case MP_EXT: > str = data; > - mp_next(&data); > + int8_t type; > + len = mp_decode_extl(&data, &type); > + if (type == MP_UUID) { > + assert(len == UUID_LEN); > + struct tt_uuid *uuid = &val[i].u.uuid; > + data = str; > + if (mp_decode_uuid(&data, uuid) == NULL) > + goto error; > + val[i].type = MEM_TYPE_UUID; > + break; > + } > + data += len; > if (mem_copy_bin(&val[i], str, data - str) != 0) > goto error; > break; 11. You need to patch memTracePrint() as well. > diff --git a/test/sql-tap/sql_uuid.c b/test/sql-tap/sql_uuid.c > new file mode 100644 > index 000000000..592b9e48f > --- /dev/null > +++ b/test/sql-tap/sql_uuid.c 12. It is already in sql* folder, no need to prefix the file name with 'sql_'. > @@ -0,0 +1,57 @@ > +#include "msgpuck.h" > +#include "module.h" > +#include "uuid/mp_uuid.h" > +#include "mp_extension_types.h" > + > +enum { > + BUF_SIZE = 512, > +}; > + > +int > +is_uuid(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + uint32_t arg_count = mp_decode_array(&args); > + if (arg_count != 1) { > + return box_error_set(__FILE__, __LINE__, ER_PROC_C, > + "invalid argument count"); > + } > + bool is_uuid; > + if (mp_typeof(*args) == MP_EXT) { > + const char *str = args; > + int8_t type; > + mp_decode_extl(&str, &type); > + is_uuid = type == MP_UUID; > + } else { > + is_uuid = false; > + } > + > + char tuple_buf[BUF_SIZE]; > + assert(mp_sizeof_array(1) + mp_sizeof_bool(is_uuid) < BUF_SIZE); > + char *d = tuple_buf; > + d = mp_encode_array(d, 1); > + d = mp_encode_bool(d, is_uuid); > + > + box_tuple_format_t *fmt = box_tuple_format_default(); > + box_tuple_t *tuple = box_tuple_new(fmt, tuple_buf, d); > + if (tuple == NULL) > + return -1; > + return box_return_tuple(ctx, tuple); 13. Use box_return_mp() instead of all that. The same below. > +} > + > +int > +ret_uuid(box_function_ctx_t *ctx, const char *args, const char *args_end) > +{ > + struct tt_uuid uuid; > + memset(&uuid, 0x11, sizeof(uuid)); > + char tuple_buf[BUF_SIZE]; > + assert(mp_sizeof_array(1) + mp_sizeof_uuid() < BUF_SIZE); > + char *d = tuple_buf; > + d = mp_encode_array(d, 1); > + d = mp_encode_uuid(d, &uuid); > + > + box_tuple_format_t *fmt = box_tuple_format_default(); > + box_tuple_t *tuple = box_tuple_new(fmt, tuple_buf, d); > + if (tuple == NULL) > + return -1; > + return box_return_tuple(ctx, tuple); > +} > diff --git a/test/sql-tap/uuid.test.lua b/test/sql-tap/uuid.test.lua > new file mode 100755 > index 000000000..9210d05db > --- /dev/null > +++ b/test/sql-tap/uuid.test.lua > @@ -0,0 +1,1259 @@ <...> > + > +-- Check that CHECK constraint can work with UUID. > +test:do_catchsql_test( > + "uuid-5.2.1", > + [[ > + INSERT INTO t5c SELECT 1, u FROM t2 LIMIT 1; > + SELECT * from t5c; 14. This SELECT is unreachable. The same in 5.3.2, 8.1.3, 8.1.4, 8.1.5, 8.1.6, 8.2.1, 8.2.3, 8.2.4, 8.2.5, 8.2.6, 8.2.8. > + ]], { > + 1, "Check constraint failed 'CK': CAST(f AS STRING) != '11111111-1111-1111-1111-111111111111'" > + }) 15. It might worth dropping the created spaces in order to clear the test artifacts.