Re: [Tarantool-patches] [PATCH v5 18/52] sql: introduce mem_concat()

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

Thank you for the review! My answers, diff and new patch below.

On Sun, Apr 11, 2021 at 08:11:53PM +0200, Vladislav Shpilevoy wrote:
> Good job on the patch!
> 
> See 2 comments below.
> 
> On 09.04.2021 19:57, Mergen Imeev via Tarantool-patches wrote:
> > This patch introduces mem_concat(). Function mem_concat() concatenates
> > values from two MEMs in case these values are strings or binaries and
> > writes the result to the third MEM.
> > 
> > Part of #5818
> > ---
> >  src/box/sql/mem.c  | 64 ++++++++++++++++++++++++++++++++++++++++++++++
> >  src/box/sql/mem.h  |  8 ++++++
> >  src/box/sql/vdbe.c | 50 ++----------------------------------
> >  3 files changed, 74 insertions(+), 48 deletions(-)
> > 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index b417c1007..2d76ef88d 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -326,6 +326,70 @@ mem_move(struct Mem *to, struct Mem *from)
> >  	return 0;
> >  }
> >  
> > +static bool
> > +is_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
> > +	       enum field_type type)
> 
> 1. Functions called 'is_*' never should change anything.
> 
Fixed. Renamed to check_result_null().

> Another question is why do you even need it? It is used in a single place,
> where it could be just inlined. And is not used in a place, where it could
> be applied.
> 
I added it here since it was the first commit, which used it. This functions
will be used in all arithmetic and bitwise operations with two operands.

> > +{
> > +	mem_clear(result);
> > +	result->field_type = type;
> > +	return (((a->flags | b->flags) & MEM_Null) != 0);
> > +}
> > +
> > +int
> > +mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
> > +{
> > +	assert(result != b);
> > +	if (a != result) {
> > +		if (is_result_null(a, b, result, FIELD_TYPE_STRING))
> > +			return 0;
> > +	} else {
> > +		if (((a->flags | b->flags) & MEM_Null) != 0) {
> > +			mem_clear(a);
> > +			result->field_type = FIELD_TYPE_STRING;
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	/* Concatenation operation can be applied only to strings and blobs. */
> > +	if ((b->flags & (MEM_Str | MEM_Blob)) == 0) {
> > +		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > +			 "text or varbinary", mem_type_to_str(b));
> > +		return -1;
> > +	}
> > +	if ((a->flags & (MEM_Str | MEM_Blob)) == 0) {
> > +		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > +			 "text or varbinary", mem_type_to_str(a));
> > +		return -1;
> > +	}
> > +
> > +	/* Moreover, both operands must be of the same type. */
> > +	if ((b->flags & MEM_Str) != (a->flags & MEM_Str)) {
> > +		diag_set(ClientError, ER_INCONSISTENT_TYPES,
> > +			 mem_type_to_str(a), mem_type_to_str(b));
> > +		return -1;
> > +	}
> > +
> > +	if (ExpandBlob(a) != 0 || ExpandBlob(b) != 0)
> > +		return -1;
> > +
> > +	uint32_t size = a->n + b->n;
> > +	if ((int)size > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
> > +		diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big");
> > +		return -1;
> > +	}
> > +	if (sqlVdbeMemGrow(result, size, result == a) != 0)
> > +		return -1;
> > +
> > +	result->flags = a->flags & (MEM_Str | MEM_Blob);
> 
> 2. Why isn't result cleared? What if it was an Agg, or Frame?
> I see before your patch they called vdbe_prepare_null_out(), which
> cleared the mem.
> 
In case result != left result is cleared in check_result_null(). In the other
case it is cleared if one of operands is NULL. If it is not cleared than
result == left, which means that if it is not varbinary of string, the error
will be returned. There shouldn't be any problem during destruction, I think.
Also, if we clear result when result == left we will get NULL as result of
concatenation no matter what left and right were. I do not know why result
was cleared previously, since it may lead to result described above.

> > +	if ((result->flags & MEM_Blob) != 0)
> > +		result->field_type = FIELD_TYPE_VARBINARY;
> > +	if (result != a)
> > +		memcpy(result->z, a->z, a->n);
> > +	memcpy(&result->z[a->n], b->z, b->n);
> > +	result->n = size;
> > +	return 0;
> > +}


Diff:


diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 67c8d90ee..f3bb62369 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -181,8 +181,8 @@ mem_move(struct Mem *to, struct Mem *from)
 }
 
 static bool
-is_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
-	       enum field_type type)
+check_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
+		  enum field_type type)
 {
 	mem_clear(result);
 	result->field_type = type;
@@ -194,7 +194,7 @@ mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
 {
 	assert(result != b);
 	if (a != result) {
-		if (is_result_null(a, b, result, FIELD_TYPE_STRING))
+		if (check_result_null(a, b, result, FIELD_TYPE_STRING))
 			return 0;
 	} else {
 		if (((a->flags | b->flags) & MEM_Null) != 0) {



New patch:


commit e95f6aea6698865e5c864fa8ae1b5393c4faf553
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Mar 13 21:30:54 2021 +0300

    sql: introduce mem_concat()
    
    This patch introduces mem_concat(). Function mem_concat() concatenates
    values from two MEMs in case these values are strings or binaries and
    writes the result to the third MEM.
    
    Part of #5818

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index a7caac014..f3bb62369 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -180,6 +180,70 @@ mem_move(struct Mem *to, struct Mem *from)
 	from->zMalloc = NULL;
 }
 
+static bool
+check_result_null(const struct Mem *a, const struct Mem *b, struct Mem *result,
+		  enum field_type type)
+{
+	mem_clear(result);
+	result->field_type = type;
+	return (((a->flags | b->flags) & MEM_Null) != 0);
+}
+
+int
+mem_concat(struct Mem *a, struct Mem *b, struct Mem *result)
+{
+	assert(result != b);
+	if (a != result) {
+		if (check_result_null(a, b, result, FIELD_TYPE_STRING))
+			return 0;
+	} else {
+		if (((a->flags | b->flags) & MEM_Null) != 0) {
+			mem_clear(a);
+			result->field_type = FIELD_TYPE_STRING;
+			return 0;
+		}
+	}
+
+	/* Concatenation operation can be applied only to strings and blobs. */
+	if ((b->flags & (MEM_Str | MEM_Blob)) == 0) {
+		diag_set(ClientError, ER_INCONSISTENT_TYPES,
+			 "text or varbinary", mem_type_to_str(b));
+		return -1;
+	}
+	if ((a->flags & (MEM_Str | MEM_Blob)) == 0) {
+		diag_set(ClientError, ER_INCONSISTENT_TYPES,
+			 "text or varbinary", mem_type_to_str(a));
+		return -1;
+	}
+
+	/* Moreover, both operands must be of the same type. */
+	if ((b->flags & MEM_Str) != (a->flags & MEM_Str)) {
+		diag_set(ClientError, ER_INCONSISTENT_TYPES,
+			 mem_type_to_str(a), mem_type_to_str(b));
+		return -1;
+	}
+
+	if (ExpandBlob(a) != 0 || ExpandBlob(b) != 0)
+		return -1;
+
+	uint32_t size = a->n + b->n;
+	if ((int)size > sql_get()->aLimit[SQL_LIMIT_LENGTH]) {
+		diag_set(ClientError, ER_SQL_EXECUTE, "string or blob too big");
+		return -1;
+	}
+	if (sqlVdbeMemGrow(result, size, result == a) != 0)
+		return -1;
+
+	result->flags = a->flags & (MEM_Str | MEM_Blob);
+	if ((result->flags & MEM_Blob) != 0)
+		result->field_type = FIELD_TYPE_VARBINARY;
+	if (result != a)
+		memcpy(result->z, a->z, a->n);
+	memcpy(&result->z[a->n], b->z, b->n);
+	result->n = size;
+	return 0;
+}
+
 static inline bool
 mem_has_msgpack_subtype(struct Mem *mem)
 {
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 37f6ac5df..d17ed0593 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -322,6 +322,14 @@ mem_copy_as_ephemeral(struct Mem *to, const struct Mem *from);
 void
 mem_move(struct Mem *to, struct Mem *from);
 
+/**
+ * Concatenate strings or binaries from the first and the second MEMs and write
+ * to the result MEM. In case the first MEM or the second MEM is NULL, the
+ * result MEM is set to NULL even if the result MEM is actually the first MEM.
+ */
+int
+mem_concat(struct Mem *left, struct Mem *right, struct Mem *result);
+
 /**
  * Simple type to str convertor. It is used to simplify
  * error reporting.
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e73ed173d..c017c5091 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1060,57 +1060,11 @@ case OP_ResultRow: {
  * types (i.e. TEXT and BLOB).
  */
 case OP_Concat: {           /* same as TK_CONCAT, in1, in2, out3 */
-	i64 nByte;
-
 	pIn1 = &aMem[pOp->p1];
 	pIn2 = &aMem[pOp->p2];
-	pOut = vdbe_prepare_null_out(p, pOp->p3);
-	assert(pIn1!=pOut);
-	if (mem_is_any_null(pIn1, pIn2)) {
-		/* Force NULL be of type STRING. */
-		pOut->field_type = FIELD_TYPE_STRING;
-		break;
-	}
-	/*
-	 * Concatenation operation can be applied only to
-	 * strings and blobs.
-	 */
-	if (!mem_is_bytes(pIn1) || !mem_is_bytes(pIn2)) {
-		char *inconsistent_type = !mem_is_bytes(pIn1) ?
-					  mem_type_to_str(pIn1) :
-					  mem_type_to_str(pIn2);
-		diag_set(ClientError, ER_INCONSISTENT_TYPES,
-			 "text or varbinary", inconsistent_type);
-		goto abort_due_to_error;
-	}
-
-	/* Moreover, both operands must be of the same type. */
-	if (!mem_is_same_type(pIn1, pIn2)) {
-		diag_set(ClientError, ER_INCONSISTENT_TYPES,
-			 mem_type_to_str(pIn2), mem_type_to_str(pIn1));
-		goto abort_due_to_error;
-	}
-	if (ExpandBlob(pIn1) != 0 || ExpandBlob(pIn2) != 0)
+	pOut = &aMem[pOp->p3];
+	if (mem_concat(pIn2, pIn1, pOut) != 0)
 		goto abort_due_to_error;
-	nByte = pIn1->n + pIn2->n;
-	if (nByte>db->aLimit[SQL_LIMIT_LENGTH]) {
-		goto too_big;
-	}
-	if (sqlVdbeMemGrow(pOut, (int)nByte+2, pOut==pIn2)) {
-		goto no_mem;
-	}
-	if (mem_is_str(pIn1))
-		MemSetTypeFlag(pOut, MEM_Str);
-	else
-		MemSetTypeFlag(pOut, MEM_Blob);
-	if (pOut!=pIn2) {
-		memcpy(pOut->z, pIn2->z, pIn2->n);
-	}
-	memcpy(&pOut->z[pIn2->n], pIn1->z, pIn1->n);
-	pOut->z[nByte]=0;
-	pOut->z[nByte+1] = 0;
-	pOut->flags |= MEM_Term;
-	pOut->n = (int)nByte;
 	UPDATE_MAX_BLOBSIZE(pOut);
 	break;
 }