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

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

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.

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.

> +{
> +	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.

> +	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;
> +}