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

Mergen Imeev imeevma at tarantool.org
Thu Jun 3 12:04:55 MSK 2021


Hi! Thank you for the review! My answers and diff below.

On Wed, Jun 02, 2021 at 11:13:00PM +0300, Timur Safin wrote:
> There are few minor complains...
> : -----Original Message-----
> : From: imeevma at tarantool.org <imeevma at tarantool.org>
> : Sent: Wednesday, June 2, 2021 11:03 AM
> : To: tsafin at tarantool.org
> : Cc: tarantool-patches at dev.tarantool.org
> : Subject: [PATCH v2 1/2] sql: introduce UUID field type
> : 
> : This patch introduces UUID to SQL. UUID is now available as a new field
> : type.
> : 
> : Part of #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.
> : 
> : According to the comparison rules, there will be no implicit casting in
> : the comparison. This rule also applies to UUID values: if a value is not
> : part of a SCALAR field, it cannot be compared to a value of any other
> : type. If the value is in a SCALAR field, it can be compared to any other
> : scalar value according to the comparison rules for a SCALAR field.
> : 
> : In case a UUID value is used in an operation that is not a comparison,
> : it can be implicitly converted to STRING or VARBINARY.
> : 
> : If a STRING or VARBINARY value is used in an operation that is not a
> : comparison, it can be implicitly converted to a UUID.
> : 
> : UUID value can always be explicitly converted to STRING or VARBINARY.
> : 
> : A STRING or VARBINARY value can be explicitly converted to a UUID if it
> : conforms to the UUID standard.
> : ---
> :  extra/mkkeywordhash.c                         |    1 +
> :  src/box/sql/func.c                            |   30 +-
> :  src/box/sql/mem.c                             |  203 ++-
> :  src/box/sql/mem.h                             |   29 +-
> :  src/box/sql/parse.y                           |    1 +
> :  src/box/sql/vdbe.c                            |   15 +-
> :  test/sql-tap/CMakeLists.txt                   |    1 +
> :  .../gh-5913-segfault-on-select-uuid.test.lua  |   42 +-
> :  .../sql-tap/gh-6024-funcs-return-bin.test.lua |    8 +-
> :  test/sql-tap/sql_uuid.c                       |   46 +
> :  test/sql-tap/uuid.test.lua                    | 1294 +++++++++++++++++
> :  11 files changed, 1602 insertions(+), 68 deletions(-)
> :  create mode 100644 test/sql-tap/sql_uuid.c
> :  create mode 100755 test/sql-tap/uuid.test.lua
> : 
> : diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
> : index 7480c0211..0d998506c 100644
> : --- a/extra/mkkeywordhash.c
> : +++ b/extra/mkkeywordhash.c
> : @@ -172,6 +172,7 @@ static Keyword aKeywordTable[] = {
> :    { "UNSIGNED",               "TK_UNSIGNED",    true  },
> :    { "UPDATE",                 "TK_UPDATE",      true  },
> :    { "USING",                  "TK_USING",       true  },
> : +  { "UUID" ,                  "TK_UUID" ,       true  },
> :    { "VALUES",                 "TK_VALUES",      true  },
> :    { "VARBINARY",              "TK_VARBINARY",   true  },
> :    { "VIEW",                   "TK_VIEW",        true  },
> : diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> : index 90e8e152f..9c4480a92 100644
> : --- a/src/box/sql/func.c
> : +++ b/src/box/sql/func.c
> : @@ -58,7 +58,8 @@ static const void *
> :  mem_as_bin(struct Mem *mem)
> :  {
> :  	const char *s;
> : -	if (!mem_is_bytes(mem) && mem_to_str(mem) != 0)
> : +	if (mem_cast_implicit(mem, FIELD_TYPE_VARBINARY) != 0 &&
> : +	    mem_to_str(mem) != 0)
> :  		return NULL;
> :  	if (mem_get_bin(mem, &s) != 0)
> :  		return NULL;
> : @@ -142,26 +143,29 @@ typeofFunc(sql_context * context, int NotUsed,
> : sql_value ** argv)
> :  				-1, SQL_STATIC);
> :  		return;
> :  	}
> : -	switch (sql_value_type(argv[0])) {
> : -	case MP_INT:
> : -	case MP_UINT:
> : +	switch (argv[0]->type) {
> : +	case MEM_TYPE_INT:
> : +	case MEM_TYPE_UINT:
> :  		z = "integer";
> :  		break;
> : -	case MP_STR:
> : +	case MEM_TYPE_STR:
> :  		z = "string";
> :  		break;
> : -	case MP_DOUBLE:
> : +	case MEM_TYPE_DOUBLE:
> :  		z = "double";
> :  		break;
> : -	case MP_BIN:
> : -	case MP_ARRAY:
> : -	case MP_MAP:
> : +	case MEM_TYPE_BIN:
> : +	case MEM_TYPE_ARRAY:
> : +	case MEM_TYPE_MAP:
> :  		z = "varbinary";
> :  		break;
> : -	case MP_BOOL:
> : -	case MP_NIL:
> : +	case MEM_TYPE_BOOL:
> : +	case MEM_TYPE_NULL:
> :  		z = "boolean";
> :  		break;
> : +	case MEM_TYPE_UUID:
> : +		z = "uuid";
> : +		break;
> :  	default:
> :  		unreachable();
> :  		break;
> : @@ -191,6 +195,7 @@ lengthFunc(sql_context * context, int argc, sql_value **
> : argv)
> :  			sql_result_uint(context, mem_len_unsafe(argv[0]));
> :  			break;
> :  		}
> : +	case MP_EXT:
> :  	case MP_STR:{
> :  			const unsigned char *z = mem_as_ustr(argv[0]);
> :  			if (z == 0)
> : @@ -235,6 +240,7 @@ absFunc(sql_context * context, int argc, sql_value **
> : argv)
> :  		}
> :  	case MP_BOOL:
> :  	case MP_BIN:
> : +	case MP_EXT:
> :  	case MP_ARRAY:
> :  	case MP_MAP: {
> :  		diag_set(ClientError, ER_INCONSISTENT_TYPES, "number",
> : @@ -1461,8 +1467,8 @@ trim_func_one_arg(struct sql_context *context,
> : sql_value *arg)
> :  		default_trim = (const unsigned char *) "\0";
> :  	else
> :  		default_trim = (const unsigned char *) " ";
> : -	int input_str_sz = mem_len_unsafe(arg);
> :  	const unsigned char *input_str = mem_as_ustr(arg);
> : +	int input_str_sz = mem_len_unsafe(arg);
> :  	uint8_t trim_char_len[1] = { 1 };
> :  	trim_procedure(context, TRIM_BOTH, default_trim, trim_char_len, 1,
> :  		       input_str, input_str_sz);
> : diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> : index 9894c09af..f0dfce395 100644
> : --- a/src/box/sql/mem.c
> : +++ b/src/box/sql/mem.c
> : @@ -58,6 +58,16 @@ enum {
> :  	BUF_SIZE = 32,
> :  };
> : 
> : +bool
> : +mem_is_field_compatible(const struct Mem *mem, enum field_type type)
> : +{
> : +	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, true);
> : +}
> : +
> :  const char *
> :  mem_str(const struct Mem *mem)
> :  {
> : @@ -81,6 +91,8 @@ mem_str(const struct Mem *mem)
> :  	case MEM_TYPE_MAP:
> :  	case MEM_TYPE_ARRAY:
> :  		return mp_str(mem->z);
> : +	case MEM_TYPE_UUID:
> : +		return tt_uuid_str(&mem->u.uuid);
> :  	case MEM_TYPE_BOOL:
> :  		return mem->u.b ? "TRUE" : "FALSE";
> :  	default:
> : @@ -190,6 +202,16 @@ mem_set_double(struct Mem *mem, double value)
> :  	mem->type = MEM_TYPE_DOUBLE;
> :  }
> : 
> : +void
> : +mem_set_uuid(struct Mem *mem, const struct tt_uuid *uuid)
> : +{
> : +	mem_clear(mem);
> : +	mem->field_type = FIELD_TYPE_UUID;
> : +	mem->u.uuid = *uuid;
> : +	mem->type = MEM_TYPE_UUID;
> : +	assert(mem->flags == 0);
> 
> What's the point of this assertion? It looks redundant, because flags 
> supposed to be nulled at the end of mem_clear()?
> (Not critical problem, but simply confusing)
> 
For me it is something that can be used as comment that says that flags should
be 0, also it checks flag in debug.

> 
> : +}
> : +
> :  static inline void
> :  set_str_const(struct Mem *mem, char *value, uint32_t len, int alloc_type)
> :  {
> : @@ -585,6 +607,18 @@ str_to_bin(struct Mem *mem)
> :  	return 0;
> :  }
> : 
> : +static inline int
> : +str_to_uuid(struct Mem *mem)
> : +{
> : +	assert(mem->type == MEM_TYPE_STR);
> : +	struct tt_uuid uuid;
> : +	if (mem->n != UUID_STR_LEN ||
> : +	    tt_uuid_from_string(tt_cstr(mem->z, mem->n), &uuid) != 0)
> 
> Eventually, when we would be able to parse more relaxed formats in tt_uuid_from_string()
> (e.g. with dashes omitted or braces around uuid) we would have to remove this check for 
> particular length UUID_STR_LEN, offloading any correctness check entirely to the function.
> I'm not insisting that this check should be modified right away. Though, from future 
> Compatible point of view it would be ideal... But not required. Just saying.
> 
> BTW, while we are here - could we avoid this static_alloc() call in tt_cstr() as a 
> prerequisite to tt_uuid_from_string(), and pass view of this data with mem->z, mem->n
> directly there?
> 
Actually, I can and I did. Fixed. Diff below.

> : +		return -1;
> : +	mem_set_uuid(mem, &uuid);
> : +	return 0;
> : +}
> : +
> :  static inline int
> :  str_to_bool(struct Mem *mem)
> :  {
> : @@ -639,6 +673,19 @@ bin_to_str0(struct Mem *mem)
> :  	return 0;
> :  }
> : 
> : +static inline int
> : +bin_to_uuid(struct Mem *mem)
> : +{
> : +	assert(mem->type == MEM_TYPE_BIN);
> : +	if (ExpandBlob(mem) != 0)
> : +		return -1;
> : +	if (mem->n != UUID_LEN ||
> : +	    tt_uuid_validate((struct tt_uuid *)mem->z) != 0)
> : +		return -1;
> : +	mem_set_uuid(mem, (struct tt_uuid *)mem->z);
> : +	return 0;
> : +}
> : +
> :  static inline int
> :  bytes_to_int(struct Mem *mem)
> :  {
> : @@ -810,6 +857,22 @@ map_to_str0(struct Mem *mem)
> :  	return mem_copy_str0(mem, str);
> :  }
> : 
> : +static inline int
> : +uuid_to_str0(struct Mem *mem)
> : +{
> : +	assert(mem->type == MEM_TYPE_UUID);
> : +	char buf[UUID_STR_LEN + 1];
> : +	tt_uuid_to_string(&mem->u.uuid, &buf[0]);
> : +	return mem_copy_str0(mem, &buf[0]);
> : +}
> : +
> : +static inline int
> : +uuid_to_bin(struct Mem *mem)
> : +{
> : +	assert(mem->type == MEM_TYPE_UUID);
> : +	return mem_copy_bin(mem, (char *)&mem->u.uuid, UUID_LEN);
> : +}
> : +
> :  int
> :  mem_to_int(struct Mem *mem)
> :  {
> : @@ -889,6 +952,8 @@ mem_to_str0(struct Mem *mem)
> :  		return map_to_str0(mem);
> :  	case MEM_TYPE_ARRAY:
> :  		return array_to_str0(mem);
> : +	case MEM_TYPE_UUID:
> : +		return uuid_to_str0(mem);
> :  	default:
> :  		return -1;
> :  	}
> : @@ -914,6 +979,8 @@ mem_to_str(struct Mem *mem)
> :  		return map_to_str0(mem);
> :  	case MEM_TYPE_ARRAY:
> :  		return array_to_str0(mem);
> : +	case MEM_TYPE_UUID:
> : +		return uuid_to_str0(mem);
> :  	default:
> :  		return -1;
> :  	}
> : @@ -966,9 +1033,19 @@ mem_cast_explicit(struct Mem *mem, enum field_type
> : type)
> :  			return str_to_bin(mem);
> :  		if (mem_is_bytes(mem))
> :  			return 0;
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return uuid_to_bin(mem);
> :  		return -1;
> :  	case FIELD_TYPE_NUMBER:
> :  		return mem_to_number(mem);
> : +	case FIELD_TYPE_UUID:
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return 0;
> : +		if (mem->type == MEM_TYPE_STR)
> : +			return str_to_uuid(mem);
> : +		if (mem->type == MEM_TYPE_BIN)
> : +			return bin_to_uuid(mem);
> : +		return -1;
> :  	case FIELD_TYPE_SCALAR:
> :  		if ((mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0)
> :  			return -1;
> : @@ -996,6 +1073,8 @@ mem_cast_implicit(struct Mem *mem, enum field_type
> : type)
> :  	case FIELD_TYPE_STRING:
> :  		if (mem->type == MEM_TYPE_STR)
> :  			return 0;
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return uuid_to_str0(mem);
> :  		return -1;
> :  	case FIELD_TYPE_DOUBLE:
> :  		if (mem->type == MEM_TYPE_DOUBLE)
> : @@ -1017,6 +1096,8 @@ mem_cast_implicit(struct Mem *mem, enum field_type
> : type)
> :  		if ((mem->type & (MEM_TYPE_BIN | MEM_TYPE_MAP |
> :  				  MEM_TYPE_ARRAY)) != 0)
> :  			return 0;
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return uuid_to_bin(mem);
> :  		return -1;
> :  	case FIELD_TYPE_NUMBER:
> :  		if (mem_is_num(mem))
> : @@ -1034,6 +1115,14 @@ mem_cast_implicit(struct Mem *mem, enum field_type
> : type)
> :  		if ((mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0)
> :  			return -1;
> :  		return 0;
> : +	case FIELD_TYPE_UUID:
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return 0;
> : +		if (mem->type == MEM_TYPE_STR)
> : +			return str_to_uuid(mem);
> : +		if (mem->type == MEM_TYPE_BIN)
> : +			return bin_to_uuid(mem);
> : +		return -1;
> :  	case FIELD_TYPE_ANY:
> :  		return 0;
> :  	default:
> : @@ -1063,6 +1152,8 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type
> : type)
> :  			return int_to_str0(mem);
> :  		if (mem->type == MEM_TYPE_DOUBLE)
> :  			return double_to_str0(mem);
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return uuid_to_str0(mem);
> :  		return -1;
> :  	case FIELD_TYPE_DOUBLE:
> :  		if (mem->type == MEM_TYPE_DOUBLE)
> : @@ -1087,6 +1178,8 @@ mem_cast_implicit_old(struct Mem *mem, enum field_type
> : type)
> :  	case FIELD_TYPE_VARBINARY:
> :  		if (mem->type == MEM_TYPE_BIN)
> :  			return 0;
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return uuid_to_bin(mem);
> :  		return -1;
> :  	case FIELD_TYPE_NUMBER:
> :  		if (mem_is_num(mem))
> : @@ -1106,6 +1199,14 @@ mem_cast_implicit_old(struct Mem *mem, enum
> : field_type type)
> :  		if ((mem->type & (MEM_TYPE_MAP | MEM_TYPE_ARRAY)) != 0)
> :  			return -1;
> :  		return 0;
> : +	case FIELD_TYPE_UUID:
> : +		if (mem->type == MEM_TYPE_UUID)
> : +			return 0;
> : +		if (mem->type == MEM_TYPE_STR)
> : +			return str_to_uuid(mem);
> : +		if (mem->type == MEM_TYPE_BIN)
> : +			return bin_to_uuid(mem);
> : +		return -1;
> :  	default:
> :  		break;
> :  	}
> 
> Uh-oh, those changes in mem_cast_implicit_old() will conflict with my
> cleanup here.But I'll handle it.
> (OTOH, could we not touch mem_cast_implicit_old as it will be deleted soon?)
> 
I believe we can't. It is easier to remove function during merge than to fix
possible inconsistency.

> : @@ -1899,6 +2000,15 @@ mem_cmp_str(const struct Mem *left, const struct Mem
> : *right, int *result,
> :  	return 0;
> :  }
> : 
> : +int
> : +mem_cmp_uuid(const struct Mem *a, const struct Mem *b, int *result)
> : +{
> : +	if ((a->type & b->type & MEM_TYPE_UUID) == 0)
> : +		return -1;
> : +	*result = memcmp(&a->u.uuid, &b->u.uuid, UUID_LEN);
> : +	return 0;
> : +}
> : +
> :  /*
> :   * Both *pMem1 and *pMem2 contain string values. Compare the two values
> :   * using the collation sequence pColl. As usual, return a negative , zero
> : @@ -1951,13 +2061,15 @@ mem_type_to_str(const struct Mem *p)
> :  		return "varbinary";
> :  	case MEM_TYPE_BOOL:
> :  		return "boolean";
> : +	case MEM_TYPE_UUID:
> : +		return "uuid";
> :  	default:
> :  		unreachable();
> :  	}
> :  }
> : 
> :  enum mp_type
> : -mem_mp_type(struct Mem *mem)
> : +mem_mp_type(const struct Mem *mem)
> :  {
> :  	assert(mem->type < MEM_TYPE_INVALID);
> :  	switch (mem->type) {
> : @@ -1979,6 +2091,8 @@ mem_mp_type(struct Mem *mem)
> :  		return MP_BOOL;
> :  	case MEM_TYPE_DOUBLE:
> :  		return MP_DOUBLE;
> : +	case MEM_TYPE_UUID:
> : +		return MP_EXT;
> :  	default:
> :  		unreachable();
> :  	}
> : @@ -2144,6 +2258,9 @@ memTracePrint(Mem *p)
> :  	case MEM_TYPE_BOOL:
> :  		printf(" bool:%s", SQL_TOKEN_BOOLEAN(p->u.b));
> :  		return;
> : +	case MEM_TYPE_UUID:
> : +		printf(" uuid:%s", tt_uuid_str(&p->u.uuid));
> : +		return;
> :  	default: {
> :  		char zBuf[200];
> :  		sqlVdbeMemPrettyPrint(p, zBuf);
> : @@ -2360,6 +2477,14 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2,
> : const struct coll * pColl)
> :  		return -1;
> :  	}
> : 
> : +	if (((type1 | type2) & MEM_TYPE_UUID) != 0) {
> : +		if (mem_cmp_uuid(pMem1, pMem2, &res) == 0)
> : +			return res;
> : +		if (type1 != MEM_TYPE_UUID)
> : +			return +1;
> : +		return -1;
> : +	}
> : +
> :  	/* At least one of the two values is a number
> :  	 */
> :  	if (((type1 | type2) &
> : @@ -2565,13 +2690,30 @@ sqlVdbeCompareMsgpack(const char **key1,
> :  			break;
> :  		}
> :  	case MP_ARRAY:
> : -	case MP_MAP:
> : -	case MP_EXT:{
> : +	case MP_MAP: {
> :  			mem1.z = (char *)aKey1;
> :  			mp_next(&aKey1);
> :  			mem1.n = aKey1 - (char *)mem1.z;
> :  			goto do_blob;
> :  		}
> : +	case MP_EXT: {
> : +		int8_t type;
> : +		const char *buf = aKey1;
> : +		uint32_t len = mp_decode_extl(&aKey1, &type);
> : +		if (type == MP_UUID) {
> : +			assert(len == UUID_LEN);
> : +			mem1.type = MEM_TYPE_UUID;
> : +			aKey1 = buf;
> : +			if (mp_decode_uuid(&aKey1, &mem1.u.uuid) == NULL ||
> : +			    mem_cmp_uuid(&mem1, pKey2, &rc) != 0)
> : +				rc = 1;
> : +			break;
> : +		}
> : +		aKey1 += len;
> : +		mem1.z = (char *)buf;
> : +		mem1.n = aKey1 - buf;
> : +		goto do_blob;
> : +	}
> :  	}
> :  	*key1 = aKey1;
> :  	return rc;
> : @@ -2625,9 +2767,25 @@ mem_from_mp_ephemeral(struct Mem *mem, const char
> : *buf, uint32_t *len)
> :  		break;
> :  	}
> :  	case MP_EXT: {
> : -		mem->z = (char *)buf;
> : -		mp_next(&buf);
> : -		mem->n = buf - mem->z;
> : +		int8_t type;
> : +		const char *svp = buf;
> : +		uint32_t size = mp_decode_extl(&buf, &type);
> : +		if (type == MP_UUID) {
> : +			assert(size == UUID_LEN);
> : +			buf = svp;
> : +			if (mp_decode_uuid(&buf, &mem->u.uuid) == NULL) {
> : +				diag_set(ClientError, ER_INVALID_MSGPACK,
> : +					 "Invalid MP_UUID MsgPack format");
> : +				return -1;
> : +			}
> : +			mem->type = MEM_TYPE_UUID;
> : +			mem->flags = 0;
> : +			mem->field_type = FIELD_TYPE_UUID;
> : +			break;
> : +		}
> : +		buf += size;
> : +		mem->z = (char *)svp;
> : +		mem->n = buf - svp;
> :  		mem->type = MEM_TYPE_BIN;
> :  		mem->flags = MEM_Ephem;
> :  		mem->field_type = FIELD_TYPE_VARBINARY;
> : @@ -2764,6 +2922,9 @@ mpstream_encode_vdbe_mem(struct mpstream *stream,
> : struct Mem *var)
> :  	case MEM_TYPE_BOOL:
> :  		mpstream_encode_bool(stream, var->u.b);
> :  		return;
> : +	case MEM_TYPE_UUID:
> : +		mpstream_encode_uuid(stream, &var->u.uuid);
> : +		return;
> :  	default:
> :  		unreachable();
> :  	}
> : @@ -2850,6 +3011,9 @@ 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:
> : +			*luaL_pushuuid(L) = mem->u.uuid;
> : +			break;
> :  		default:
> :  			unreachable();
> :  		}
> : @@ -2976,14 +3140,8 @@ port_lua_get_vdbemem(struct port *base, uint32_t
> : *size)
> :  			uint32_t size;
> :  			uint32_t svp = region_used(&fiber()->gc);
> :  			if (field.ext_type == MP_UUID) {
> : -				size = mp_sizeof_uuid();
> : -				buf = region_alloc(&fiber()->gc, size);
> : -				if (buf == NULL) {
> : -					diag_set(OutOfMemory, size,
> : -						 "region_alloc", "buf");
> : -					goto error;
> : -				}
> : -				mp_encode_uuid(buf, field.uuidval);
> : +				mem_set_uuid(&val[i], field.uuidval);
> : +				break;
> :  			} else {
> :  				size = mp_sizeof_decimal(field.decval);
> :  				buf = region_alloc(&fiber()->gc, size);
> : @@ -3087,7 +3245,22 @@ 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) {
> : +					diag_set(ClientError,
> : +						 ER_INVALID_MSGPACK, "Invalid "
> : +						 "MP_UUID MsgPack format");
> : +					goto error;
> : +				}
> : +				val[i].type = MEM_TYPE_UUID;
> : +				break;
> : +			}
> : +			data += len;
> :  			if (mem_copy_bin(&val[i], str, data - str) != 0)
> :  				goto error;
> :  			break;
> : diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
> : index 15d97da0e..b3cd5c545 100644
> : --- a/src/box/sql/mem.h
> : +++ b/src/box/sql/mem.h
> : @@ -30,6 +30,7 @@
> :   * SUCH DAMAGE.
> :   */
> :  #include "box/field_def.h"
> : +#include "uuid/tt_uuid.h"
> : 
> :  struct sql;
> :  struct Vdbe;
> : @@ -47,10 +48,11 @@ enum mem_type {
> :  	MEM_TYPE_MAP		= 1 << 6,
> :  	MEM_TYPE_BOOL		= 1 << 7,
> :  	MEM_TYPE_DOUBLE		= 1 << 8,
> : -	MEM_TYPE_INVALID	= 1 << 9,
> : -	MEM_TYPE_FRAME		= 1 << 10,
> : -	MEM_TYPE_PTR		= 1 << 11,
> : -	MEM_TYPE_AGG		= 1 << 12,
> : +	MEM_TYPE_UUID		= 1 << 9,
> : +	MEM_TYPE_INVALID	= 1 << 10,
> : +	MEM_TYPE_FRAME		= 1 << 11,
> : +	MEM_TYPE_PTR		= 1 << 12,
> : +	MEM_TYPE_AGG		= 1 << 13,
> :  };
> : 
> :  /*
> : @@ -72,6 +74,7 @@ struct Mem {
> :  		 */
> :  		struct func *func;
> :  		struct VdbeFrame *pFrame;	/* Used when flags==MEM_Frame */
> : +		struct tt_uuid uuid;
> :  	} u;
> :  	/** Type of the value this MEM contains. */
> :  	enum mem_type type;
> : @@ -255,6 +258,10 @@ mem_is_any_null(const struct Mem *mem1, const struct
> : Mem *mem2)
> :  	return ((mem1->type| mem2->type) & MEM_TYPE_NULL) != 0;
> :  }
> : 
> : +/** Check if MEM is compatible with field type. */
> : +bool
> : +mem_is_field_compatible(const struct Mem *mem, enum field_type type);
> : +
> :  /**
> :   * Return a string that represent content of MEM. String is either
> : allocated
> :   * using static_alloc() of just a static variable.
> : @@ -290,6 +297,10 @@ mem_set_bool(struct Mem *mem, bool value);
> :  void
> :  mem_set_double(struct Mem *mem, double value);
> : 
> : +/** Clear MEM and set it to UUID. */
> : +void
> : +mem_set_uuid(struct Mem *mem, const struct tt_uuid *uuid);
> : +
> :  /** Clear MEM and set it to STRING. The string belongs to another object.
> : */
> :  void
> :  mem_set_str_ephemeral(struct Mem *mem, char *value, uint32_t len);
> : @@ -677,6 +688,14 @@ mem_cmp_str(const struct Mem *left, const struct Mem
> : *right, int *result,
> :  int
> :  mem_cmp_num(const struct Mem *a, const struct Mem *b, int *result);
> : 
> : +/**
> : + * Compare two MEMs and return the result of comparison. MEMs should be of
> : + * UUID type or their values are converted to UUID according to
> : + * implicit cast rules. Original MEMs are not changed.
> : + */
> : +int
> : +mem_cmp_uuid(const struct Mem *left, const struct Mem *right, int *result);
> : +
> :  /**
> :   * Convert the given MEM to INTEGER. This function and the function below
> : define
> :   * the rules that are used to convert values of all other types to INTEGER.
> : In
> : @@ -898,7 +917,7 @@ mem_type_to_str(const struct Mem *p);
> :   * transparent memory cell.
> :   */
> :  enum mp_type
> : -mem_mp_type(struct Mem *mem);
> : +mem_mp_type(const struct Mem *mem);
> : 
> :  enum mp_type
> :  sql_value_type(struct Mem *);
> : diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> : index abc363951..4c9cf475e 100644
> : --- a/src/box/sql/parse.y
> : +++ b/src/box/sql/parse.y
> : @@ -1834,6 +1834,7 @@ typedef(A) ::= SCALAR . { A.type = FIELD_TYPE_SCALAR;
> : }
> :  typedef(A) ::= BOOL . { A.type = FIELD_TYPE_BOOLEAN; }
> :  typedef(A) ::= BOOLEAN . { A.type = FIELD_TYPE_BOOLEAN; }
> :  typedef(A) ::= VARBINARY . { A.type = FIELD_TYPE_VARBINARY; }
> : +typedef(A) ::= UUID . { A.type = FIELD_TYPE_UUID; }
> : 
> :  /**
> :   * Time-like types are temporary disabled, until they are
> : diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> : index 12ec703a2..32d02d96e 100644
> : --- a/src/box/sql/vdbe.c
> : +++ b/src/box/sql/vdbe.c
> : @@ -1322,10 +1322,10 @@ case OP_FunctionByName: {
> :  	region_truncate(region, region_svp);
> :  	if (mem == NULL)
> :  		goto abort_due_to_error;
> : -	enum mp_type type = sql_value_type((sql_value *)pOut);
> : -	if (!field_mp_plain_type_is_compatible(returns, type, true)) {
> : +	if (!mem_is_field_compatible(pOut, returns)) {
> :  		diag_set(ClientError, ER_FUNC_INVALID_RETURN_TYPE, pOp->p4.z,
> : -			 field_type_strs[returns], mp_type_strs[type]);
> : +			 field_type_strs[returns],
> : +			 mp_type_strs[mem_mp_type(pOut)]);
> :  		goto abort_due_to_error;
> :  	}
> : 
> : @@ -1634,6 +1634,15 @@ case OP_Ge: {             /* same as TK_GE, jump,
> : in1, in3 */
> :  				 "boolean");
> :  			goto abort_due_to_error;
> :  		}
> : +	} else if (((pIn3->type | pIn1->type) & MEM_TYPE_UUID) != 0) {
> : +		if (mem_cmp_uuid(pIn3, pIn1, &res) != 0) {
> : +			char *str = pIn3->type != MEM_TYPE_UUID ?
> : +				    mem_type_to_str(pIn3) :
> : +				    mem_type_to_str(pIn1);
> : +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
> : +				 "uuid");
> : +			goto abort_due_to_error;
> : +		}
> :  	} else if (mem_is_bin(pIn3) || mem_is_bin(pIn1)) {
> :  		if (mem_cmp_bin(pIn3, pIn1, &res) != 0) {
> :  			char *str = !mem_is_bin(pIn3) ?
> 
> In general, despite minor complains, it all looks good to me, and ok to commit.
> Those complains we will address later.
> 
> LGTM
> 
> Timur
> 

Diff:


diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index f0dfce395..6f3bf52e5 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -612,8 +612,7 @@ str_to_uuid(struct Mem *mem)
 {
 	assert(mem->type == MEM_TYPE_STR);
 	struct tt_uuid uuid;
-	if (mem->n != UUID_STR_LEN ||
-	    tt_uuid_from_string(tt_cstr(mem->z, mem->n), &uuid) != 0)
+	if (tt_uuid_from_strl(mem->z, mem->n, &uuid) != 0)
 		return -1;
 	mem_set_uuid(mem, &uuid);
 	return 0;


More information about the Tarantool-patches mailing list