Re: [Tarantool-patches] [PATCH 1/1] sql: introduce UUID field type

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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.