[Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions

[Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions

[Mergen Imeev via Tarantool-patches]

This patch adds proper initialization for the MEM, which is used in the
aggregate functions min() and max().

Part of #4906
---
 src/box/sql/func.c    | 21 ++++++++++-----------
 src/box/sql/sqlInt.h  |  7 +++++++
 src/box/sql/vdbeapi.c | 20 ++++++++++++++++++++
 3 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 9c28d5122..d282b2cea 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1766,14 +1766,14 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 
 	struct func_sql_builtin *func =
 		(struct func_sql_builtin *)context->func;
-	pBest = (Mem *) sql_aggregate_context(context, sizeof(*pBest));
+	pBest = sql_context_agg_mem(context);
 	if (!pBest)
 		return;
 
 	if (mem_is_null(argv[0])) {
-		if (pBest->flags)
+		if (!mem_is_null(pBest))
 			sqlSkipAccumulatorLoad(context);
-	} else if (pBest->flags) {
+	} else if (!mem_is_null(pBest)) {
 		int cmp;
 		struct coll *pColl = sqlGetFuncCollSeq(context);
 		/*
@@ -1798,14 +1798,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
 static void
 minMaxFinalize(sql_context * context)
 {
-	sql_value *pRes;
-	pRes = (sql_value *) sql_aggregate_context(context, 0);
-	if (pRes) {
-		if (pRes->flags) {
-			sql_result_value(context, pRes);
-		}
-		mem_destroy(pRes);
-	}
+	struct Mem *mem = context->pMem;
+	struct Mem *res;
+	if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0)
+		return;
+	if (!mem_is_null(res))
+		sql_result_value(context, res);
+	mem_destroy(res);
 }
 
 /*
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b548ddad4..ef8dcd693 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -484,6 +484,13 @@ void *
 sql_aggregate_context(sql_context *,
 			  int nBytes);
 
+/**
+ * Allocate or return the aggregate context containing struct MEM for a user
+ * function. A new context is allocated on the first call. Subsequent calls
+ * return the same context that was returned on prior calls.
+ */
+struct Mem *
+sql_context_agg_mem(struct sql_context *context);
 
 int
 sql_column_count(sql_stmt * pStmt);
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 655743fb1..aaae12e41 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -394,6 +394,26 @@ sql_aggregate_context(sql_context * p, int nByte)
 	return accum;
 }
 
+struct Mem *
+sql_context_agg_mem(struct sql_context *ctx)
+{
+	assert(ctx != NULL && ctx->func != NULL);
+	assert(ctx->func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
+	assert(ctx->func->def->aggregate == FUNC_AGGREGATE_GROUP);
+	struct Mem *mem;
+	if (!mem_is_agg(ctx->pMem)) {
+		if (mem_set_agg(ctx->pMem, ctx->func, sizeof(*mem)) != 0)
+			return NULL;
+		if (mem_get_agg(ctx->pMem, (void **)&mem) != 0)
+			return NULL;
+		mem_create(mem);
+		return mem;
+	}
+	if (mem_get_agg(ctx->pMem, (void **)&mem) != 0)
+		return NULL;
+	return mem;
+}
+
 /*
  * Return the number of columns in the result set for the statement pStmt.
  */
-- 
2.25.1

[Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY

[Mergen Imeev via Tarantool-patches]

After this patch, the mem_is_bin() function will return 'true' only if
the value that the MEM contains is of type VARBINARY. This patch also
adds the mem_is_bin_ext() function, which is used to check if a MEM
contains value of type VARBINARY or value of types that are currently
considered VARBINARY extensions - MAP and ARRAY.

Part of #4906
---
 src/box/sql/func.c | 10 +++++-----
 src/box/sql/mem.h  |  8 +++++++-
 src/box/sql/vdbe.c | 16 ++++++++--------
 3 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index d282b2cea..bed7e8488 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -553,7 +553,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 	if (mem_is_null(argv[0]))
 		return;
-	if (mem_is_bin(argv[0])) {
+	if (mem_is_bin_ext(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
@@ -613,7 +613,7 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
 	const char *z2;                                                        \
 	int n;                                                                 \
 	UNUSED_PARAMETER(argc);                                                \
-	if (mem_is_bin(argv[0])) {                                             \
+	if (mem_is_bin_ext(argv[0])) {                                         \
 		diag_set(ClientError, ER_INCONSISTENT_TYPES, "text",           \
 			 "varbinary");                                         \
 		context->is_aborted = true;                                    \
@@ -694,7 +694,7 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
 	unsigned char *p;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	if (mem_is_bin(argv[0])) {
+	if (mem_is_bin_ext(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
@@ -1455,7 +1455,7 @@ trim_func_one_arg(struct sql_context *context, sql_value *arg)
 	const unsigned char *default_trim;
 	if (mem_is_null(arg))
 		return;
-	if (mem_is_bin(arg))
+	if (mem_is_bin_ext(arg))
 		default_trim = (const unsigned char *) "\0";
 	else
 		default_trim = (const unsigned char *) " ";
@@ -1584,7 +1584,7 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 		1, 2, 6, 2, 3, 0, 1, 0, 2, 0, 2, 0, 0, 0, 0, 0,
 	};
 	assert(argc == 1);
-	if (mem_is_bin(argv[0])) {
+	if (mem_is_bin_ext(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "text");
 		context->is_aborted = true;
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 1db7f4deb..6fc15617d 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -184,7 +184,7 @@ mem_is_bool(const struct Mem *mem)
 static inline bool
 mem_is_bin(const struct Mem *mem)
 {
-	return (mem->flags & MEM_Blob) != 0;
+	return (mem->flags & MEM_Blob) != 0 && (mem->flags & MEM_Subtype) == 0;
 }
 
 static inline bool
@@ -206,6 +206,12 @@ mem_is_array(const struct Mem *mem)
 	       mp_typeof(*mem->z) == MP_ARRAY;
 }
 
+static inline bool
+mem_is_bin_ext(const struct Mem *mem)
+{
+	return (mem->flags & MEM_Blob) != 0;
+}
+
 static inline bool
 mem_is_agg(const struct Mem *mem)
 {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2308587e7..bedfa87af 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1634,9 +1634,9 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 				 "boolean");
 			goto abort_due_to_error;
 		}
-	} else if (mem_is_bin(pIn3) || mem_is_bin(pIn1)) {
+	} else if (mem_is_bin_ext(pIn3) || mem_is_bin_ext(pIn1)) {
 		if (mem_cmp_bin(pIn3, pIn1, &res) != 0) {
-			char *str = !mem_is_bin(pIn3) ?
+			char *str = !mem_is_bin_ext(pIn3) ?
 				    mem_type_to_str(pIn3) :
 				    mem_type_to_str(pIn1);
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
@@ -2991,7 +2991,7 @@ case OP_Found: {        /* jump, in3 */
 	} else {
 		pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def);
 		if (pIdxKey==0) goto no_mem;
-		assert(mem_is_bin(pIn3));
+		assert(mem_is_bin_ext(pIn3));
 		(void)ExpandBlob(pIn3);
 		sqlVdbeRecordUnpackMsgpack(pC->key_def,
 					       pIn3->z, pIdxKey);
@@ -3253,7 +3253,7 @@ case OP_SorterData: {
 	assert(isSorter(pC));
 	if (sqlVdbeSorterRowkey(pC, pOut) != 0)
 		goto abort_due_to_error;
-	assert(mem_is_bin(pOut));
+	assert(mem_is_bin_ext(pOut));
 	assert(pOp->p1>=0 && pOp->p1<p->nCursor);
 	p->apCsr[pOp->p3]->cacheStatus = CACHE_STALE;
 	break;
@@ -3616,7 +3616,7 @@ case OP_SorterInsert: {      /* in2 */
 	assert(cursor != NULL);
 	assert(isSorter(cursor));
 	pIn2 = &aMem[pOp->p2];
-	assert(mem_is_bin(pIn2));
+	assert(mem_is_bin_ext(pIn2));
 	if (ExpandBlob(pIn2) != 0 ||
 	    sqlVdbeSorterWrite(cursor, pIn2) != 0)
 		goto abort_due_to_error;
@@ -3650,7 +3650,7 @@ case OP_SorterInsert: {      /* in2 */
 case OP_IdxReplace:
 case OP_IdxInsert: {
 	pIn2 = &aMem[pOp->p1];
-	assert(mem_is_bin(pIn2));
+	assert(mem_is_bin_ext(pIn2));
 	if (ExpandBlob(pIn2) != 0)
 		goto abort_due_to_error;
 	struct space *space;
@@ -3741,10 +3741,10 @@ case OP_Update: {
 	assert(pOp->p4type == P4_SPACEPTR);
 
 	struct Mem *key_mem = &aMem[pOp->p2];
-	assert(mem_is_bin(key_mem));
+	assert(mem_is_bin_ext(key_mem));
 
 	struct Mem *upd_fields_mem = &aMem[pOp->p3];
-	assert(mem_is_bin(upd_fields_mem));
+	assert(mem_is_bin_ext(upd_fields_mem));
 	uint32_t *upd_fields = (uint32_t *)upd_fields_mem->z;
 	uint32_t upd_fields_cnt = upd_fields_mem->n / sizeof(uint32_t);
 
-- 
2.25.1

Re: [Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions

[Vladislav Shpilevoy via Tarantool-patches]

Hi! Thanks for the patch!

> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 9c28d5122..d282b2cea 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -1798,14 +1798,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
>  static void
>  minMaxFinalize(sql_context * context)
>  {
> -	sql_value *pRes;
> -	pRes = (sql_value *) sql_aggregate_context(context, 0);
> -	if (pRes) {
> -		if (pRes->flags) {
> -			sql_result_value(context, pRes);
> -		}
> -		mem_destroy(pRes);
> -	}
> +	struct Mem *mem = context->pMem;
> +	struct Mem *res;
> +	if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0)

You don't need `is_agg` check. `get` does the same already.

> +		return;
> +	if (!mem_is_null(res))
> +		sql_result_value(context, res);
> +	mem_destroy(res);
>  }

Re: [Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY

[Vladislav Shpilevoy via Tarantool-patches]

Thanks for working on this!

See 11 comments below!

On 27.04.2021 18:55, Mergen Imeev via Tarantool-patches wrote:
> After this patch, the mem_is_bin() function will return 'true' only if
> the value that the MEM contains is of type VARBINARY. This patch also
> adds the mem_is_bin_ext() function, which is used to check if a MEM
> contains value of type VARBINARY or value of types that are currently
> considered VARBINARY extensions - MAP and ARRAY.
> 
> Part of #4906
> ---
>  src/box/sql/func.c | 10 +++++-----
>  src/box/sql/mem.h  |  8 +++++++-
>  src/box/sql/vdbe.c | 16 ++++++++--------
>  3 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index d282b2cea..bed7e8488 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -553,7 +553,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
>  	}
>  	if (mem_is_null(argv[0]))
>  		return;
> -	if (mem_is_bin(argv[0])) {
> +	if (mem_is_bin_ext(argv[0])) {

1. The name is quite confusing. I spent some time thinking about a
better one, but can't find any. Maybe because such a function looks
very strange in its purpose, as well as some of its usages.

Maybe better add a function mem_is_nested() to check for MP_ARRAY
and MP_MAP. Or mem_is_scalar(), which checks it is not MP_ARRAY and
not MP_MAP and you call it with negation. Or add special checks
for array and map. mem_is_bin_ext() would be either

	mem_is_bin() || mem_is_nested()

or

	mem_is_bin() || !mem_is_scalar()

or

	mem_is_bin() || mem_is_array() || mem_is_map()

Then you would see something strange below.

Why in ROUND() we check for types we do not accept instead of
checking the types we do accept? I mean, why not

	if (!mem_is_number() && !mem_is_str())
		return error;

instead of

	if (mem_is_bin_ext())
		return error;

What will happen when you will add UUID, date? Will you change
this on each new type like that?:

	if (mem_is_bin_ext() || mem_is_uuid() || mem_is_date() || ...)
		return error;

IMO would be easier to maintain a list of supported types than a
list of not supported types. Worth changing now?

>  		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
>  			 mem_str(argv[0]), "numeric");
>  		context->is_aborted = true;
> @@ -613,7 +613,7 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
>  	const char *z2;                                                        \
>  	int n;                                                                 \
>  	UNUSED_PARAMETER(argc);                                                \
> -	if (mem_is_bin(argv[0])) {                                             \
> +	if (mem_is_bin_ext(argv[0])) {                                         \

2. Ditto.

>  		diag_set(ClientError, ER_INCONSISTENT_TYPES, "text",           \
>  			 "varbinary");                                         \
>  		context->is_aborted = true;                                    \
> @@ -694,7 +694,7 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
>  	unsigned char *p;
>  	assert(argc == 1);
>  	UNUSED_PARAMETER(argc);
> -	if (mem_is_bin(argv[0])) {
> +	if (mem_is_bin_ext(argv[0])) {

3. Ditto.

>  		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
>  			 mem_str(argv[0]), "numeric");
>  		context->is_aborted = true;
> @@ -1455,7 +1455,7 @@ trim_func_one_arg(struct sql_context *context, sql_value *arg)
>  	const unsigned char *default_trim;
>  	if (mem_is_null(arg))
>  		return;
> -	if (mem_is_bin(arg))
> +	if (mem_is_bin_ext(arg))

4. TBH, I would ban MP_ARRAY and MP_MAP from TRIM(). It makes 0
sense for them. I am not sure it is even tested. So we are not
talking about a notable 'behaviour change' really.

>  		default_trim = (const unsigned char *) "\0";
>  	else
>  		default_trim = (const unsigned char *) " ";
> @@ -1584,7 +1584,7 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
>  		1, 2, 6, 2, 3, 0, 1, 0, 2, 0, 2, 0, 0, 0, 0, 0,
>  	};
>  	assert(argc == 1);
> -	if (mem_is_bin(argv[0])) {
> +	if (mem_is_bin_ext(argv[0])) {

5. Ditto.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 2308587e7..bedfa87af 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1634,9 +1634,9 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
>  				 "boolean");
>  			goto abort_due_to_error;
>  		}
> -	} else if (mem_is_bin(pIn3) || mem_is_bin(pIn1)) {
> +	} else if (mem_is_bin_ext(pIn3) || mem_is_bin_ext(pIn1)) {
>  		if (mem_cmp_bin(pIn3, pIn1, &res) != 0) {

6. Arrays and maps are not comparable by memcmp(). The same numbers
can be encoded in MessagePack as 8 bytes and as 1 byte (you can do that
legally, just not widely used). In 2 maps elements might be in different
order. And obviously you can't compare map and array. Such places would
draw more attention for future fixes if we wouldn't mask them behind
_ext() suffix but would rather use more explicit checks.

> -			char *str = !mem_is_bin(pIn3) ?
> +			char *str = !mem_is_bin_ext(pIn3) ?
>  				    mem_type_to_str(pIn3) :
>  				    mem_type_to_str(pIn1);
>  			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
> @@ -2991,7 +2991,7 @@ case OP_Found: {        /* jump, in3 */
>  	} else {
>  		pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def);
>  		if (pIdxKey==0) goto no_mem;
> -		assert(mem_is_bin(pIn3));
> +		assert(mem_is_bin_ext(pIn3));

7. This would be mem_is_array() AFAIU. Because it is a tuple, right?

>  		(void)ExpandBlob(pIn3);
>  		sqlVdbeRecordUnpackMsgpack(pC->key_def,
>  					       pIn3->z, pIdxKey);
> @@ -3253,7 +3253,7 @@ case OP_SorterData: {
>  	assert(isSorter(pC));
>  	if (sqlVdbeSorterRowkey(pC, pOut) != 0)
>  		goto abort_due_to_error;
> -	assert(mem_is_bin(pOut));
> +	assert(mem_is_bin_ext(pOut));

8. Ditto.

>  	assert(pOp->p1>=0 && pOp->p1<p->nCursor);
>  	p->apCsr[pOp->p3]->cacheStatus = CACHE_STALE;
>  	break;
> @@ -3616,7 +3616,7 @@ case OP_SorterInsert: {      /* in2 */
>  	assert(cursor != NULL);
>  	assert(isSorter(cursor));
>  	pIn2 = &aMem[pOp->p2];
> -	assert(mem_is_bin(pIn2));
> +	assert(mem_is_bin_ext(pIn2));

9. Ditto.

>  	if (ExpandBlob(pIn2) != 0 ||
>  	    sqlVdbeSorterWrite(cursor, pIn2) != 0)
>  		goto abort_due_to_error;
> @@ -3650,7 +3650,7 @@ case OP_SorterInsert: {      /* in2 */
>  case OP_IdxReplace:
>  case OP_IdxInsert: {
>  	pIn2 = &aMem[pOp->p1];
> -	assert(mem_is_bin(pIn2));
> +	assert(mem_is_bin_ext(pIn2));

10. Ditto.

>  	if (ExpandBlob(pIn2) != 0)
>  		goto abort_due_to_error;
>  	struct space *space;
> @@ -3741,10 +3741,10 @@ case OP_Update: {
>  	assert(pOp->p4type == P4_SPACEPTR);
>  
>  	struct Mem *key_mem = &aMem[pOp->p2];
> -	assert(mem_is_bin(key_mem));
> +	assert(mem_is_bin_ext(key_mem));

11. Ditto here and below.

>  	struct Mem *upd_fields_mem = &aMem[pOp->p3];
> -	assert(mem_is_bin(upd_fields_mem));
> +	assert(mem_is_bin_ext(upd_fields_mem));
>  	uint32_t *upd_fields = (uint32_t *)upd_fields_mem->z;
>  	uint32_t upd_fields_cnt = upd_fields_mem->n / sizeof(uint32_t);

Re: [Tarantool-patches] [PATCH v2 1/3] sql: initialize MEM used in aggregate functions

[Mergen Imeev via Tarantool-patches]

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

On Thu, Apr 29, 2021 at 11:02:06PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index 9c28d5122..d282b2cea 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -1798,14 +1798,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
> >  static void
> >  minMaxFinalize(sql_context * context)
> >  {
> > -	sql_value *pRes;
> > -	pRes = (sql_value *) sql_aggregate_context(context, 0);
> > -	if (pRes) {
> > -		if (pRes->flags) {
> > -			sql_result_value(context, pRes);
> > -		}
> > -		mem_destroy(pRes);
> > -	}
> > +	struct Mem *mem = context->pMem;
> > +	struct Mem *res;
> > +	if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0)
> 
> You don't need `is_agg` check. `get` does the same already.
> 
Fixed.

> > +		return;
> > +	if (!mem_is_null(res))
> > +		sql_result_value(context, res);
> > +	mem_destroy(res);
> >  }

Diff:


diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index d282b2cea..94302cc56 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1800,7 +1800,7 @@ minMaxFinalize(sql_context * context)
 {
 	struct Mem *mem = context->pMem;
 	struct Mem *res;
-	if (!mem_is_agg(mem) || mem_get_agg(mem, (void **)&res) != 0)
+	if (mem_get_agg(mem, (void **)&res) != 0)
 		return;
 	if (!mem_is_null(res))
 		sql_result_value(context, res);

Re: [Tarantool-patches] [PATCH v2 2/3] sql: make mem_is_bin() to check only for VARBINARY

[Mergen Imeev via Tarantool-patches]

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

On Thu, Apr 29, 2021 at 11:05:19PM +0200, Vladislav Shpilevoy wrote:
> Thanks for working on this!
> 
> See 11 comments below!
> 
> On 27.04.2021 18:55, Mergen Imeev via Tarantool-patches wrote:
> > After this patch, the mem_is_bin() function will return 'true' only if
> > the value that the MEM contains is of type VARBINARY. This patch also
> > adds the mem_is_bin_ext() function, which is used to check if a MEM
> > contains value of type VARBINARY or value of types that are currently
> > considered VARBINARY extensions - MAP and ARRAY.
> > 
> > Part of #4906
> > ---
> >  src/box/sql/func.c | 10 +++++-----
> >  src/box/sql/mem.h  |  8 +++++++-
> >  src/box/sql/vdbe.c | 16 ++++++++--------
> >  3 files changed, 20 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> > index d282b2cea..bed7e8488 100644
> > --- a/src/box/sql/func.c
> > +++ b/src/box/sql/func.c
> > @@ -553,7 +553,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
> >  	}
> >  	if (mem_is_null(argv[0]))
> >  		return;
> > -	if (mem_is_bin(argv[0])) {
> > +	if (mem_is_bin_ext(argv[0])) {
> 
> 1. The name is quite confusing. I spent some time thinking about a
> better one, but can't find any. Maybe because such a function looks
> very strange in its purpose, as well as some of its usages.
> 
> Maybe better add a function mem_is_nested() to check for MP_ARRAY
> and MP_MAP. Or mem_is_scalar(), which checks it is not MP_ARRAY and
> not MP_MAP and you call it with negation. Or add special checks
> for array and map. mem_is_bin_ext() would be either
> 
> 	mem_is_bin() || mem_is_nested()
> 
> or
> 
> 	mem_is_bin() || !mem_is_scalar()
> 
> or
> 
> 	mem_is_bin() || mem_is_array() || mem_is_map()
> 
Fixed. However, it turned out that you suggestion made diff even less, though
you suggested to use two or three functions instead of one.

> Then you would see something strange below.
> 
> Why in ROUND() we check for types we do not accept instead of
> checking the types we do accept? I mean, why not
> 
> 	if (!mem_is_number() && !mem_is_str())
> 		return error;
> 
> instead of
> 
> 	if (mem_is_bin_ext())
> 		return error;
> 
> What will happen when you will add UUID, date? Will you change
> this on each new type like that?:
> 
> 	if (mem_is_bin_ext() || mem_is_uuid() || mem_is_date() || ...)
> 		return error;
> 
> IMO would be easier to maintain a list of supported types than a
> list of not supported types. Worth changing now?
> 
I changed here, however I do not think that it should be done this way, since
we have to take into account our implicit cast rules and change this code every
time we will add something, that can be converted to value of accepted type.
Instead, all check and convertions should be done before the function is called.
This will be done in issue #4159, which was already partially done last year.
Last time I wasn't able to complete this issue, but we were able to come to a
conclusion about how should it be done. I think I will return to the issue in
Q3 or Q4.

> >  		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> >  			 mem_str(argv[0]), "numeric");
> >  		context->is_aborted = true;
> > @@ -613,7 +613,7 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
> >  	const char *z2;                                                        \
> >  	int n;                                                                 \
> >  	UNUSED_PARAMETER(argc);                                                \
> > -	if (mem_is_bin(argv[0])) {                                             \
> > +	if (mem_is_bin_ext(argv[0])) {                                         \
> 
> 2. Ditto.
> 
Fixed.

> >  		diag_set(ClientError, ER_INCONSISTENT_TYPES, "text",           \
> >  			 "varbinary");                                         \
> >  		context->is_aborted = true;                                    \
> > @@ -694,7 +694,7 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
> >  	unsigned char *p;
> >  	assert(argc == 1);
> >  	UNUSED_PARAMETER(argc);
> > -	if (mem_is_bin(argv[0])) {
> > +	if (mem_is_bin_ext(argv[0])) {
> 
> 3. Ditto.
> 
Fixed.

> >  		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> >  			 mem_str(argv[0]), "numeric");
> >  		context->is_aborted = true;
> > @@ -1455,7 +1455,7 @@ trim_func_one_arg(struct sql_context *context, sql_value *arg)
> >  	const unsigned char *default_trim;
> >  	if (mem_is_null(arg))
> >  		return;
> > -	if (mem_is_bin(arg))
> > +	if (mem_is_bin_ext(arg))
> 
> 4. TBH, I would ban MP_ARRAY and MP_MAP from TRIM(). It makes 0
> sense for them. I am not sure it is even tested. So we are not
> talking about a notable 'behaviour change' really.
> 
In current design TRIM() accepts any argument. I reverted this change, however
it should not affect MAP and ARRAY values.

> >  		default_trim = (const unsigned char *) "\0";
> >  	else
> >  		default_trim = (const unsigned char *) " ";
> > @@ -1584,7 +1584,7 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
> >  		1, 2, 6, 2, 3, 0, 1, 0, 2, 0, 2, 0, 0, 0, 0, 0,
> >  	};
> >  	assert(argc == 1);
> > -	if (mem_is_bin(argv[0])) {
> > +	if (mem_is_bin_ext(argv[0])) {
> 
> 5. Ditto.
> 
Fixed.

> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 2308587e7..bedfa87af 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -1634,9 +1634,9 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
> >  				 "boolean");
> >  			goto abort_due_to_error;
> >  		}
> > -	} else if (mem_is_bin(pIn3) || mem_is_bin(pIn1)) {
> > +	} else if (mem_is_bin_ext(pIn3) || mem_is_bin_ext(pIn1)) {
> >  		if (mem_cmp_bin(pIn3, pIn1, &res) != 0) {
> 
> 6. Arrays and maps are not comparable by memcmp(). The same numbers
> can be encoded in MessagePack as 8 bytes and as 1 byte (you can do that
> legally, just not widely used). In 2 maps elements might be in different
> order. And obviously you can't compare map and array. Such places would
> draw more attention for future fixes if we wouldn't mask them behind
> _ext() suffix but would rather use more explicit checks.
> 
I added an error on attempt to compare MAP or ARRAY to anything.

> > -			char *str = !mem_is_bin(pIn3) ?
> > +			char *str = !mem_is_bin_ext(pIn3) ?
> >  				    mem_type_to_str(pIn3) :
> >  				    mem_type_to_str(pIn1);
> >  			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
> > @@ -2991,7 +2991,7 @@ case OP_Found: {        /* jump, in3 */
> >  	} else {
> >  		pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def);
> >  		if (pIdxKey==0) goto no_mem;
> > -		assert(mem_is_bin(pIn3));
> > +		assert(mem_is_bin_ext(pIn3));
> 
> 7. This would be mem_is_array() AFAIU. Because it is a tuple, right?
> 
It is true, that MEM contains MP_ARRAY. However, currently this MEM has
VARBINARY type. I tried to change type to ARRAY, but found unexpected for me
error: ARRAY cannot be inserted in SCALAR field, but VARBINARY can. Also,
we cannot create an index that contains MAP or ARRAY.It means that after
changing type to ARRAY, value created by OP_MakeRecord cannot be inserted to
most of ephemeral spaces, since all their fields have VARBINARY type and
their PK consist of all fields. I decided to drop my attempt and accept that
currently all internally created arrays are of VARBINARY type. Also, this
means that arrays and maps have undefined behaviour in our code, since
their behaviour depends on the source of the value.

So, for now most of maps and arrays we can just use mem_is_bin(), at least
for now.

I think that we have to rework out ephemeral spaces if we want to introduce
support of MAP and ARRAY in SQL.

> >  		(void)ExpandBlob(pIn3);
> >  		sqlVdbeRecordUnpackMsgpack(pC->key_def,
> >  					       pIn3->z, pIdxKey);
> > @@ -3253,7 +3253,7 @@ case OP_SorterData: {
> >  	assert(isSorter(pC));
> >  	if (sqlVdbeSorterRowkey(pC, pOut) != 0)
> >  		goto abort_due_to_error;
> > -	assert(mem_is_bin(pOut));
> > +	assert(mem_is_bin_ext(pOut));
> 
> 8. Ditto.
> 
Reverted.

> >  	assert(pOp->p1>=0 && pOp->p1<p->nCursor);
> >  	p->apCsr[pOp->p3]->cacheStatus = CACHE_STALE;
> >  	break;
> > @@ -3616,7 +3616,7 @@ case OP_SorterInsert: {      /* in2 */
> >  	assert(cursor != NULL);
> >  	assert(isSorter(cursor));
> >  	pIn2 = &aMem[pOp->p2];
> > -	assert(mem_is_bin(pIn2));
> > +	assert(mem_is_bin_ext(pIn2));
> 
> 9. Ditto.
> 
Reverted.

> >  	if (ExpandBlob(pIn2) != 0 ||
> >  	    sqlVdbeSorterWrite(cursor, pIn2) != 0)
> >  		goto abort_due_to_error;
> > @@ -3650,7 +3650,7 @@ case OP_SorterInsert: {      /* in2 */
> >  case OP_IdxReplace:
> >  case OP_IdxInsert: {
> >  	pIn2 = &aMem[pOp->p1];
> > -	assert(mem_is_bin(pIn2));
> > +	assert(mem_is_bin_ext(pIn2));
> 
> 10. Ditto.
> 
Reverted.

> >  	if (ExpandBlob(pIn2) != 0)
> >  		goto abort_due_to_error;
> >  	struct space *space;
> > @@ -3741,10 +3741,10 @@ case OP_Update: {
> >  	assert(pOp->p4type == P4_SPACEPTR);
> >  
> >  	struct Mem *key_mem = &aMem[pOp->p2];
> > -	assert(mem_is_bin(key_mem));
> > +	assert(mem_is_bin_ext(key_mem));
> 
> 11. Ditto here and below.
> 
Reverted.

> >  	struct Mem *upd_fields_mem = &aMem[pOp->p3];
> > -	assert(mem_is_bin(upd_fields_mem));
> > +	assert(mem_is_bin_ext(upd_fields_mem));
> >  	uint32_t *upd_fields = (uint32_t *)upd_fields_mem->z;
> >  	uint32_t upd_fields_cnt = upd_fields_mem->n / sizeof(uint32_t);


Diff:

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index ae5ead67e..90e8e152f 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -553,7 +553,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 	if (mem_is_null(argv[0]))
 		return;
-	if (mem_is_bin_ext(argv[0])) {
+	if (!mem_is_num(argv[0]) && !mem_is_str(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
@@ -613,7 +613,8 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
 	const char *z2;                                                        \
 	int n;                                                                 \
 	UNUSED_PARAMETER(argc);                                                \
-	if (mem_is_bin_ext(argv[0])) {                                         \
+	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||                      \
+	    mem_is_array(argv[0])) {                                           \
 		diag_set(ClientError, ER_INCONSISTENT_TYPES, "text",           \
 			 "varbinary");                                         \
 		context->is_aborted = true;                                    \
@@ -694,7 +695,8 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
 	unsigned char *p;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	if (mem_is_bin_ext(argv[0])) {
+	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||
+	    mem_is_array(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
@@ -1455,7 +1457,7 @@ trim_func_one_arg(struct sql_context *context, sql_value *arg)
 	const unsigned char *default_trim;
 	if (mem_is_null(arg))
 		return;
-	if (mem_is_bin_ext(arg))
+	if (mem_is_bin(arg))
 		default_trim = (const unsigned char *) "\0";
 	else
 		default_trim = (const unsigned char *) " ";
@@ -1584,7 +1586,8 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 		1, 2, 6, 2, 3, 0, 1, 0, 2, 0, 2, 0, 0, 0, 0, 0,
 	};
 	assert(argc == 1);
-	if (mem_is_bin_ext(argv[0])) {
+	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||
+	    mem_is_array(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "text");
 		context->is_aborted = true;
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 6fc15617d..526b6bf3e 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -206,12 +206,6 @@ mem_is_array(const struct Mem *mem)
 	       mp_typeof(*mem->z) == MP_ARRAY;
 }
 
-static inline bool
-mem_is_bin_ext(const struct Mem *mem)
-{
-	return (mem->flags & MEM_Blob) != 0;
-}
-
 static inline bool
 mem_is_agg(const struct Mem *mem)
 {
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index bedfa87af..12ec703a2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1634,15 +1634,20 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 				 "boolean");
 			goto abort_due_to_error;
 		}
-	} else if (mem_is_bin_ext(pIn3) || mem_is_bin_ext(pIn1)) {
+	} else if (mem_is_bin(pIn3) || mem_is_bin(pIn1)) {
 		if (mem_cmp_bin(pIn3, pIn1, &res) != 0) {
-			char *str = !mem_is_bin_ext(pIn3) ?
+			char *str = !mem_is_bin(pIn3) ?
 				    mem_type_to_str(pIn3) :
 				    mem_type_to_str(pIn1);
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, str,
 				 "varbinary");
 			goto abort_due_to_error;
 		}
+	} else if (mem_is_map(pIn3) || mem_is_map(pIn1) || mem_is_array(pIn3) ||
+		   mem_is_array(pIn1)) {
+		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+			 mem_type_to_str(pIn3), mem_type_to_str(pIn1));
+		goto abort_due_to_error;
 	} else if (type == FIELD_TYPE_STRING) {
 		if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
 			const char *str =
@@ -2991,7 +2996,7 @@ case OP_Found: {        /* jump, in3 */
 	} else {
 		pFree = pIdxKey = sqlVdbeAllocUnpackedRecord(db, pC->key_def);
 		if (pIdxKey==0) goto no_mem;
-		assert(mem_is_bin_ext(pIn3));
+		assert(mem_is_bin(pIn3));
 		(void)ExpandBlob(pIn3);
 		sqlVdbeRecordUnpackMsgpack(pC->key_def,
 					       pIn3->z, pIdxKey);
@@ -3253,7 +3258,7 @@ case OP_SorterData: {
 	assert(isSorter(pC));
 	if (sqlVdbeSorterRowkey(pC, pOut) != 0)
 		goto abort_due_to_error;
-	assert(mem_is_bin_ext(pOut));
+	assert(mem_is_bin(pOut));
 	assert(pOp->p1>=0 && pOp->p1<p->nCursor);
 	p->apCsr[pOp->p3]->cacheStatus = CACHE_STALE;
 	break;
@@ -3616,7 +3621,7 @@ case OP_SorterInsert: {      /* in2 */
 	assert(cursor != NULL);
 	assert(isSorter(cursor));
 	pIn2 = &aMem[pOp->p2];
-	assert(mem_is_bin_ext(pIn2));
+	assert(mem_is_bin(pIn2));
 	if (ExpandBlob(pIn2) != 0 ||
 	    sqlVdbeSorterWrite(cursor, pIn2) != 0)
 		goto abort_due_to_error;
@@ -3650,7 +3655,7 @@ case OP_SorterInsert: {      /* in2 */
 case OP_IdxReplace:
 case OP_IdxInsert: {
 	pIn2 = &aMem[pOp->p1];
-	assert(mem_is_bin_ext(pIn2));
+	assert(mem_is_bin(pIn2));
 	if (ExpandBlob(pIn2) != 0)
 		goto abort_due_to_error;
 	struct space *space;
@@ -3741,10 +3746,10 @@ case OP_Update: {
 	assert(pOp->p4type == P4_SPACEPTR);
 
 	struct Mem *key_mem = &aMem[pOp->p2];
-	assert(mem_is_bin_ext(key_mem));
+	assert(mem_is_bin(key_mem));
 
 	struct Mem *upd_fields_mem = &aMem[pOp->p3];
-	assert(mem_is_bin_ext(upd_fields_mem));
+	assert(mem_is_bin(upd_fields_mem));
 	uint32_t *upd_fields = (uint32_t *)upd_fields_mem->z;
 	uint32_t upd_fields_cnt = upd_fields_mem->n / sizeof(uint32_t);
 


Patch:

commit f6ad3ac1c548a2ffbfbe4b103b533f48db6458fa
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Tue Apr 27 15:12:33 2021 +0300

    sql: make mem_is_bin() to check only for VARBINARY
    
    After this patch, the mem_is_bin() function will return 'true' only if
    the value that the MEM contains is of type VARBINARY. This patch also
    adds the mem_is_bin_ext() function, which is used to check if a MEM
    contains value of type VARBINARY or value of types that are currently
    considered VARBINARY extensions - MAP and ARRAY.
    
    Part of #4906

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 94302cc56..90e8e152f 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -553,7 +553,7 @@ roundFunc(sql_context * context, int argc, sql_value ** argv)
 	}
 	if (mem_is_null(argv[0]))
 		return;
-	if (mem_is_bin(argv[0])) {
+	if (!mem_is_num(argv[0]) && !mem_is_str(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
@@ -613,7 +613,8 @@ case_type##ICUFunc(sql_context *context, int argc, sql_value **argv)   \
 	const char *z2;                                                        \
 	int n;                                                                 \
 	UNUSED_PARAMETER(argc);                                                \
-	if (mem_is_bin(argv[0])) {                                             \
+	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||                      \
+	    mem_is_array(argv[0])) {                                           \
 		diag_set(ClientError, ER_INCONSISTENT_TYPES, "text",           \
 			 "varbinary");                                         \
 		context->is_aborted = true;                                    \
@@ -694,7 +695,8 @@ randomBlob(sql_context * context, int argc, sql_value ** argv)
 	unsigned char *p;
 	assert(argc == 1);
 	UNUSED_PARAMETER(argc);
-	if (mem_is_bin(argv[0])) {
+	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||
+	    mem_is_array(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "numeric");
 		context->is_aborted = true;
@@ -1584,7 +1586,8 @@ soundexFunc(sql_context * context, int argc, sql_value ** argv)
 		1, 2, 6, 2, 3, 0, 1, 0, 2, 0, 2, 0, 0, 0, 0, 0,
 	};
 	assert(argc == 1);
-	if (mem_is_bin(argv[0])) {
+	if (mem_is_bin(argv[0]) || mem_is_map(argv[0]) ||
+	    mem_is_array(argv[0])) {
 		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 			 mem_str(argv[0]), "text");
 		context->is_aborted = true;
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 1db7f4deb..526b6bf3e 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -184,7 +184,7 @@ mem_is_bool(const struct Mem *mem)
 static inline bool
 mem_is_bin(const struct Mem *mem)
 {
-	return (mem->flags & MEM_Blob) != 0;
+	return (mem->flags & MEM_Blob) != 0 && (mem->flags & MEM_Subtype) == 0;
 }
 
 static inline bool
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 2308587e7..12ec703a2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1643,6 +1643,11 @@ case OP_Ge: {             /* same as TK_GE, jump, in1, in3 */
 				 "varbinary");
 			goto abort_due_to_error;
 		}
+	} else if (mem_is_map(pIn3) || mem_is_map(pIn1) || mem_is_array(pIn3) ||
+		   mem_is_array(pIn1)) {
+		diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
+			 mem_type_to_str(pIn3), mem_type_to_str(pIn1));
+		goto abort_due_to_error;
 	} else if (type == FIELD_TYPE_STRING) {
 		if (mem_cmp_str(pIn3, pIn1, &res, pOp->p4.pColl) != 0) {
 			const char *str =