Re: [Tarantool-patches] [PATCH v5 44/52] sql: introduce mem_cast_implicit()

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Thank you for the review! My answer below. I also included patch since there
were some changes due to conflicts with patches 'sql: introduce mem_cmp_*()
functions' and patch 'sql: introduce mem_ecast_explicit()'.


On Wed, Apr 14, 2021 at 12:59:58AM +0200, Vladislav Shpilevoy wrote:
> Good job on the fixes!
> 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 29634841a..72a84e80d 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -2092,23 +2092,12 @@ case OP_ApplyType: {
> >  	while((type = *(types++)) != field_type_MAX) {
> >  		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> >  		assert(memIsValid(pIn1));
> > -		if (!mem_is_type_compatible(pIn1, type)) {
> > -			/* Implicit cast is allowed only to numeric type. */
> > -			if (!sql_type_is_numeric(type))
> > -				goto type_mismatch;
> > -			/* Implicit cast is allowed only from numeric type. */
> > -			if (!mem_is_num(pIn1))
> > -				goto type_mismatch;
> > -			/* Try to convert numeric-to-numeric. */
> > -			if (mem_cast_explicit(pIn1, type) != 0)
> > -				goto type_mismatch;
> > +		if (mem_cast_implicit(pIn1, type) != 0) {
> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > +				 mem_str(pIn1), field_type_strs[type]);
> > +			goto abort_due_to_error;
> 
> I think it would be better both for explicit and implicit casts
> to have the diag_set() inside.
I am not sure about this. There are some cases when result of execution is not
checked, which means that diag_set() will be set without actual error. Is this
fine? If so, I have no problem with moving diag_set() to mem_cast_*() functions
and mem_to_*() functions.


New patch:


commit 10dd667c04574a05fe0e75f0d4fd9b0327df76b1
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Mar 17 12:43:08 2021 +0300

    sql: introduce mem_cast_implicit()
    
    This patch introduces mem_cast_implicit(). This function is used to
    convert a MEM to given type according to implicit cast rules.
    
    Part of #5818

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 4b619b032..537288c14 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -670,6 +670,32 @@ double_to_int_precise(struct Mem *mem)
 	return -1;
 }
 
+static inline int
+double_to_uint(struct Mem *mem)
+{
+	double d = mem->u.r;
+	if (d >= 0 && d < (double)UINT64_MAX) {
+		mem->u.u = (uint64_t)d;
+		mem->flags = MEM_UInt;
+		mem->field_type = FIELD_TYPE_UNSIGNED;
+		return 0;
+	}
+	return -1;
+}
+
+static inline int
+double_to_uint_precise(struct Mem *mem)
+{
+	double d = mem->u.r;
+	if (d >= 0 && d < (double)UINT64_MAX && (double)(uint64_t)d == d) {
+		mem->u.u = (uint64_t)d;
+		mem->flags = MEM_UInt;
+		mem->field_type = FIELD_TYPE_UNSIGNED;
+		return 0;
+	}
+	return -1;
+}
+
 static inline int
 double_to_str0(struct Mem *mem)
 {
@@ -882,6 +908,140 @@ mem_cast_explicit(struct Mem *mem, enum field_type type)
 	return -1;
 }
 
+int
+mem_cast_implicit(struct Mem *mem, enum field_type type)
+{
+	if ((mem->flags & MEM_Null) != 0) {
+		mem->field_type = type;
+		return 0;
+	}
+	switch (type) {
+	case FIELD_TYPE_UNSIGNED:
+		if ((mem->flags & MEM_UInt) != 0)
+			return 0;
+		if ((mem->flags & MEM_Real) != 0)
+			return double_to_uint(mem);
+		return -1;
+	case FIELD_TYPE_STRING:
+		if ((mem->flags & MEM_Str) != 0)
+			return 0;
+		return -1;
+	case FIELD_TYPE_DOUBLE:
+		if ((mem->flags & MEM_Real) != 0)
+			return 0;
+		if ((mem->flags & (MEM_Int | MEM_UInt)) != 0)
+			return int_to_double(mem);
+		return -1;
+	case FIELD_TYPE_INTEGER:
+		if ((mem->flags & (MEM_Int | MEM_UInt)) != 0)
+			return 0;
+		if ((mem->flags & MEM_Real) != 0)
+			return double_to_int(mem);
+		return -1;
+	case FIELD_TYPE_BOOLEAN:
+		if ((mem->flags & MEM_Bool) != 0)
+			return 0;
+		return -1;
+	case FIELD_TYPE_VARBINARY:
+		if ((mem->flags & MEM_Blob) != 0)
+			return 0;
+		return -1;
+	case FIELD_TYPE_NUMBER:
+		if ((mem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0)
+			return 0;
+		return -1;
+	case FIELD_TYPE_MAP:
+		if (mem_is_map(mem))
+			return 0;
+		return -1;
+	case FIELD_TYPE_ARRAY:
+		if (mem_is_array(mem))
+			return 0;
+		return -1;
+	case FIELD_TYPE_SCALAR:
+		if ((mem->flags & MEM_Blob) != 0 &&
+		    (mem->flags & MEM_Subtype) != 0)
+			return -1;
+		return 0;
+	case FIELD_TYPE_ANY:
+		return 0;
+	default:
+		break;
+	}
+	return -1;
+}
+
+int
+mem_cast_implicit_old(struct Mem *mem, enum field_type type)
+{
+	if (mem_is_null(mem))
+		return 0;
+	switch (type) {
+	case FIELD_TYPE_UNSIGNED:
+		if ((mem->flags & MEM_UInt) != 0)
+			return 0;
+		if ((mem->flags & MEM_Real) != 0)
+			return double_to_uint_precise(mem);
+		if ((mem->flags & MEM_Str) != 0)
+			return bytes_to_uint(mem);
+		return -1;
+	case FIELD_TYPE_STRING:
+		if ((mem->flags & (MEM_Str | MEM_Blob)) != 0)
+			return 0;
+		if ((mem->flags & (MEM_Int | MEM_UInt)) != 0)
+			return int_to_str0(mem);
+		if ((mem->flags & MEM_Real) != 0)
+			return double_to_str0(mem);
+		return -1;
+	case FIELD_TYPE_DOUBLE:
+		if ((mem->flags & MEM_Real) != 0)
+			return 0;
+		if ((mem->flags & (MEM_Int | MEM_UInt)) != 0)
+			return int_to_double(mem);
+		if ((mem->flags & MEM_Str) != 0)
+			return bin_to_str(mem);
+		return -1;
+	case FIELD_TYPE_INTEGER:
+		if ((mem->flags & (MEM_Int | MEM_UInt)) != 0)
+			return 0;
+		if ((mem->flags & MEM_Str) != 0)
+			return bytes_to_int(mem);
+		if ((mem->flags & MEM_Real) != 0)
+			return double_to_int_precise(mem);
+		return -1;
+	case FIELD_TYPE_BOOLEAN:
+		if ((mem->flags & MEM_Bool) != 0)
+			return 0;
+		return -1;
+	case FIELD_TYPE_VARBINARY:
+		if ((mem->flags & MEM_Blob) != 0)
+			return 0;
+		return -1;
+	case FIELD_TYPE_NUMBER:
+		if ((mem->flags & (MEM_Int | MEM_UInt | MEM_Real)) != 0)
+			return 0;
+		if ((mem->flags & MEM_Str) != 0)
+			return mem_to_number(mem);
+		return -1;
+	case FIELD_TYPE_MAP:
+		if (mem_is_map(mem))
+			return 0;
+		return -1;
+	case FIELD_TYPE_ARRAY:
+		if (mem_is_array(mem))
+			return 0;
+		return -1;
+	case FIELD_TYPE_SCALAR:
+		if ((mem->flags & MEM_Blob) != 0 &&
+		    (mem->flags & MEM_Subtype) != 0)
+			return -1;
+		return 0;
+	default:
+		break;
+	}
+	return -1;
+}
+
 int
 mem_copy(struct Mem *to, const struct Mem *from)
 {
@@ -1924,122 +2084,6 @@ sqlVdbeMemExpandBlob(Mem * pMem)
 	return 0;
 }
 
-/*
- * Exported version of mem_apply_type(). This one works on sql_value*,
- * not the internal Mem* type.
- */
-void
-sql_value_apply_type(
-	sql_value *pVal,
-	enum field_type type)
-{
-	mem_apply_type((Mem *) pVal, type);
-}
-
-int
-mem_apply_type(struct Mem *record, enum field_type type)
-{
-	if ((record->flags & MEM_Null) != 0)
-		return 0;
-	assert(type < field_type_MAX);
-	switch (type) {
-	case FIELD_TYPE_INTEGER:
-	case FIELD_TYPE_UNSIGNED:
-		if ((record->flags & (MEM_Bool | MEM_Blob)) != 0)
-			return -1;
-		if ((record->flags & MEM_UInt) == MEM_UInt)
-			return 0;
-		if ((record->flags & MEM_Real) == MEM_Real) {
-			double d = record->u.r;
-			if (d >= 0) {
-				if (double_compare_uint64(d, UINT64_MAX,
-							  1) > 0)
-					return 0;
-				if ((double)(uint64_t)d == d) {
-					record->u.u = (uint64_t)d;
-					record->flags = MEM_UInt;
-					record->field_type =
-						FIELD_TYPE_UNSIGNED;
-				}
-			} else {
-				if (double_compare_nint64(d, INT64_MIN, 1) < 0)
-					return 0;
-				if ((double)(int64_t)d == d) {
-					record->u.i = (int64_t)d;
-					record->flags = MEM_Int;
-					record->field_type = FIELD_TYPE_INTEGER;
-				}
-			}
-			return 0;
-		}
-		if ((record->flags & MEM_Str) != 0) {
-			bool is_neg;
-			int64_t i;
-			if (sql_atoi64(record->z, &i, &is_neg, record->n) != 0)
-				return -1;
-			mem_set_int(record, i, is_neg);
-		}
-		if ((record->flags & MEM_Int) == MEM_Int) {
-			if (type == FIELD_TYPE_UNSIGNED)
-				return -1;
-			return 0;
-		}
-		return 0;
-	case FIELD_TYPE_BOOLEAN:
-		if ((record->flags & MEM_Bool) == MEM_Bool)
-			return 0;
-		return -1;
-	case FIELD_TYPE_NUMBER:
-		if ((record->flags & (MEM_Real | MEM_Int | MEM_UInt)) != 0)
-			return 0;
-		return mem_to_double(record);
-	case FIELD_TYPE_DOUBLE:
-		if ((record->flags & MEM_Real) != 0)
-			return 0;
-		return mem_to_double(record);
-	case FIELD_TYPE_STRING:
-		/*
-		 * Only attempt the conversion to TEXT if there is
-		 * an integer or real representation (BLOB and
-		 * NULL do not get converted).
-		 */
-		if ((record->flags & MEM_Str) == 0 &&
-		    (record->flags & (MEM_Real | MEM_Int | MEM_UInt)) != 0)
-			mem_to_str(record);
-		record->flags &= ~(MEM_Real | MEM_Int | MEM_UInt);
-		return 0;
-	case FIELD_TYPE_VARBINARY:
-		if ((record->flags & MEM_Blob) == 0)
-			return -1;
-		return 0;
-	case FIELD_TYPE_SCALAR:
-		/* Can't cast MAP and ARRAY to scalar types. */
-		if ((record->flags & MEM_Subtype) != 0 &&
-		    record->subtype == SQL_SUBTYPE_MSGPACK) {
-			assert(mp_typeof(*record->z) == MP_MAP ||
-			       mp_typeof(*record->z) == MP_ARRAY);
-			return -1;
-		}
-		return 0;
-	case FIELD_TYPE_MAP:
-		if ((record->flags & MEM_Subtype) != 0 &&
-		    record->subtype == SQL_SUBTYPE_MSGPACK &&
-		    mp_typeof(*record->z) == MP_MAP)
-			return 0;
-		return -1;
-	case FIELD_TYPE_ARRAY:
-		if ((record->flags & MEM_Subtype) != 0 &&
-		    record->subtype == SQL_SUBTYPE_MSGPACK &&
-		    mp_typeof(*record->z) == MP_ARRAY)
-			return 0;
-		return -1;
-	case FIELD_TYPE_ANY:
-		return 0;
-	default:
-		return -1;
-	}
-}
-
 static int
 sqlVdbeMemGrow(struct Mem *pMem, int n, int bPreserve)
 {
@@ -2459,14 +2503,6 @@ sqlMemCompare(const Mem * pMem1, const Mem * pMem2, const struct coll * pColl)
 	return res;
 }
 
-bool
-mem_is_type_compatible(struct Mem *mem, enum field_type type)
-{
-	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);
-}
-
 int
 sql_vdbemem_finalize(struct Mem *mem, struct func *func)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 7cced5537..91c1c464f 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -736,6 +736,16 @@ mem_to_str0(struct Mem *mem);
 int
 mem_cast_explicit(struct Mem *mem, enum field_type type);
 
+/** Convert the given MEM to given type according to implicit cast rules. */
+int
+mem_cast_implicit(struct Mem *mem, enum field_type type);
+
+/**
+ * Convert the given MEM to given type according to legacy implicit cast rules.
+ */
+int
+mem_cast_implicit_old(struct Mem *mem, enum field_type type);
+
 /**
  * Simple type to str convertor. It is used to simplify
  * error reporting.
@@ -772,44 +782,6 @@ registerTrace(int iReg, Mem *p);
 int sqlVdbeMemNulTerminate(struct Mem *);
 int sqlVdbeMemExpandBlob(struct Mem *);
 #define ExpandBlob(P) (((P)->flags&MEM_Zero)?sqlVdbeMemExpandBlob(P):0)
-void sql_value_apply_type(struct Mem *val, enum field_type type);
-
-
-/**
- * Processing is determined by the field type parameter:
- *
- * INTEGER:
- *    If memory holds floating point value and it can be
- *    converted without loss (2.0 - > 2), it's type is
- *    changed to INT. Otherwise, simply return success status.
- *
- * NUMBER:
- *    If memory holds INT or floating point value,
- *    no actions take place.
- *
- * STRING:
- *    Convert mem to a string representation.
- *
- * SCALAR:
- *    Mem is unchanged, but flag is set to BLOB in case of
- *    scalar-like type. Otherwise, (MAP, ARRAY) conversion
- *    is impossible.
- *
- * BOOLEAN:
- *    If memory holds BOOLEAN no actions take place.
- *
- * ANY:
- *    Mem is unchanged, no actions take place.
- *
- * MAP/ARRAY:
- *    These types can't be casted to scalar ones, or to each
- *    other. So the only valid conversion is to type itself.
- *
- * @param record The value to apply type to.
- * @param type The type to be applied.
- */
-int
-mem_apply_type(struct Mem *record, enum field_type type);
 
 /** Setters = Change MEM value. */
 
@@ -874,17 +846,6 @@ int sqlVdbeMemTooBig(Mem *);
 
 int sqlMemCompare(const Mem *, const Mem *, const struct coll *);
 
-/**
- * Check that MEM_type of the mem is compatible with given type.
- *
- * @param mem The MEM that contains the value to check.
- * @param type The type to check.
- * @retval TRUE if the MEM_type of the value and the given type
- *         are compatible, FALSE otherwise.
- */
-bool
-mem_is_type_compatible(struct Mem *mem, enum field_type type);
-
 /** MEM manipulate functions. */
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 049965bd0..f7b6df0d9 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1645,8 +1645,9 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 		}
 	} else if (type == FIELD_TYPE_STRING) {
 		if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
-			const char *str = mem_apply_type(pIn3, type) != 0 ?
-					  mem_str(pIn3) : mem_str(pIn1);
+			const char *str =
+				mem_cast_implicit_old(pIn3, type) != 0 ?
+				mem_str(pIn3) : mem_str(pIn1);
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
 				 "string");
 			goto abort_due_to_error;
@@ -1655,8 +1656,9 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 		   mem_is_num(pIn1)) {
 		type = FIELD_TYPE_NUMBER;
 		if (mem_cmp_num(pIn3, pIn1, &res) != 0) {
-			const char *str = mem_apply_type(pIn3, type) != 0 ?
-					  mem_str(pIn3) : mem_str(pIn1);
+			const char *str =
+				mem_cast_implicit_old(pIn3, type) != 0 ?
+				mem_str(pIn3) : mem_str(pIn1);
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
 				 "numeric");
 			goto abort_due_to_error;
@@ -1665,8 +1667,9 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 		type = FIELD_TYPE_STRING;
 		assert(mem_is_str(pIn3) && mem_is_same_type(pIn3, pIn1));
 		if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
-			const char *str = mem_apply_type(pIn3, type) != 0 ?
-					  mem_str(pIn3) : mem_str(pIn1);
+			const char *str =
+				mem_cast_implicit_old(pIn3, type) != 0 ?
+				mem_str(pIn3) : mem_str(pIn1);
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
 				 "string");
 			goto abort_due_to_error;
@@ -2137,23 +2140,12 @@ case OP_ApplyType: {
 	while((type = *(types++)) != field_type_MAX) {
 		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
 		assert(memIsValid(pIn1));
-		if (!mem_is_type_compatible(pIn1, type)) {
-			/* Implicit cast is allowed only to numeric type. */
-			if (!sql_type_is_numeric(type))
-				goto type_mismatch;
-			/* Implicit cast is allowed only from numeric type. */
-			if (!mem_is_num(pIn1))
-				goto type_mismatch;
-			/* Try to convert numeric-to-numeric. */
-			if (mem_cast_explicit(pIn1, type) != 0)
-				goto type_mismatch;
+		if (mem_cast_implicit(pIn1, type) != 0) {
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+				 mem_str(pIn1), field_type_strs[type]);
+			goto abort_due_to_error;
 		}
 		pIn1++;
-		continue;
-type_mismatch:
-		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-			 mem_str(pIn1), field_type_strs[type]);
-		goto abort_due_to_error;
 	}
 	break;
 }
@@ -2212,7 +2204,7 @@ case OP_MakeRecord: {
 	if (types != NULL) {
 		pRec = pData0;
 		do {
-			mem_apply_type(pRec++, *(types++));
+			mem_cast_implicit_old(pRec++, *(types++));
 		} while(types[0] != field_type_MAX);
 	}
 
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 749a3b455..ec413feb8 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2334,7 +2334,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
 			sql_value *pRet = sqlValueNew(v->db);
 			if (pRet) {
 				mem_copy(pRet, pMem);
-				sql_value_apply_type(pRet, aff);
+				mem_cast_implicit_old(pRet, aff);
 			}
 			return pRet;
 		}