[Tarantool-patches] [PATCH 1/1] sql: introduce UUID field type
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sat May 22 19:04:15 MSK 2021
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.
More information about the Tarantool-patches
mailing list