[tarantool-patches] Re: [PATCH 2/2] sql: make aggregate functions types more strict

["i.koptelov" <ivan.koptelov@tarantool.org>]

> On 24 Apr 2019, at 20:37, n.pettik <korablev@tarantool.org> wrote:
> 
> 
>>>> +		if (sql_type != ref_sql_type) {
>>>> +			is_compatible = false;
>>>> +			if ((sql_type == SQL_INTEGER || sql_type == SQL_FLOAT) &&
>>>> +			    (ref_sql_type == SQL_INTEGER ||
>>>> +			     ref_sql_type == SQL_FLOAT)) {
>>>> +				is_compatible = true;
>>> 
>>> This is a very hot path and doing so much work to check
>>> compatibility is a) clumsy when reading b) slow c) hard to
>>> maintain.
>>> 
>>> Please use a compatibility matrix statically defined as a 8x8
>>> bitmap.
>> Fixed:
> 
> It is not what was suggested. Look at field_type1_contains_type2() and
> field_type_compatibility as examples.
Thank you for noticing.

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 0738f44e6..d98c60914 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1605,6 +1599,25 @@ countFinalize(sql_context * context)
 	sql_result_int64(context, p ? p->n : 0);
 }
 
+/**
+ * Table of aggregate functions args type compatibility.
+ */
+static const bool scalar_type_compatibility[] = {
+	      /*  SQL_INTEGER  SQL_FLOAT  SQL_TEXT  SQL_BLOB  SQL_NULL */
+/* SQL_INTEGER */ true,        true,      false,    false,    false,
+/* SQL_FLOAT   */ true,        true,      false,    false,    false,
+/* SQL_TEXT    */ false,       false,     true,     false,    false,
+/* SQL_BLOB    */ false,       false,     false,    true,     false,
+/* SQL_NULL    */ false,       false,     false,    false,    true,
+};
+
+static bool
+are_scalar_types_compatible(enum sql_type type1, enum sql_type type2)
+{
+	int idx = (type2 - 1) * (SQL_TYPE_MAX - 1) + (type1 - 1);
+	return scalar_type_compatibility[idx];
+}
+

> 
>> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
>> index c84f22caf..553c4f225 100644
>> --- a/src/box/sql/vdbeInt.h
>> +++ b/src/box/sql/vdbeInt.h
>> @@ -228,6 +228,7 @@ struct Mem {
>> #define MEM_Str       0x0002	/* Value is a string */
>> #define MEM_Int       0x0004	/* Value is an integer */
>> #define MEM_Real      0x0008	/* Value is a real number */
>> +#define MEM_NumMask   0x000c	/* Value is integer or real (mask) */
> 
> This mask covers neither Blob nor Bool values.
Can you please explain why it should? Blob and Bool are not numeric as far as I understand.

> 
>> #define MEM_Blob      0x0010	/* Value is a BLOB */
>> #define MEM_Bool      0x0020    /* Value is a bool */
>> #define MEM_Ptr       0x0040	/* Value is a generic pointer */
>> @@ -262,11 +263,25 @@ enum {
>> 	MEM_PURE_TYPE_MASK = 0x1f
>> };
>> 
>> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
>> index 624083b22..dd87c5b70 100644
>> --- a/src/box/sql/select.c
>> +++ b/src/box/sql/select.c
>> @@ -4351,7 +4351,7 @@ pushDownWhereTerms(Parse * pParse,        /* Parse context (for malloc() and error repo
>> * The requirement of column type not being SCALAR follows from
>> * the purpose of the function. The purpose of the function is
>> * to answer the question: "Should MIN/MAX call be optimised by
>> - * using ORDER ON clause code?" If the type of column is SCALAR
>> + * using ORDER BY clause code?" If the type of column is SCALAR
>> * then we have to iterate over all rows to check if their types
>> * are compatible. Hence, the optimisation should not be used.
>> 
>>> 
>>>> + * then we have to iterate over all rows to check if their types
>>>> + * are compatible. Hence, the optimisation should not be used.
>>>> + * For details please see: https://github.com/tarantool/tarantool/issues/4032
>>>> + *
>>>> * Or, if the conditions above are not met, *ppMinMax is set to 0 and
>>>> * WHERE_ORDERBY_NORMAL is returned.
>>>> */
>>>> @@ -4364,6 +4373,8 @@ minMaxQuery(AggInfo * pAggInfo, ExprList ** ppMinMax)
>>>> 		if (pEList && pEList->nExpr == 1
>>>> 		    && pEList->a[0].pExpr->op == TK_AGG_COLUMN) {
>>>> 			const char *zFunc = pExpr->u.zToken;
>>>> +			if (sql_expr_type(pEList->a[0].pExpr) == FIELD_TYPE_SCALAR)
>>>> +				return eRet;
>>> 
>>> I see no difference between byte code generated for SCALAR and other
>>> columns (SELECT MIN(a) FROM t;). Investigate this case please.
>>> 
>> I am not quite understand what is the problem with this.
>> If the following conditions are true, then the result of MIN/MAX is retrieved without
>> usage of corresponding function in func:
>> 
>> * the query contains just a single aggregate function,
>> * the aggregate function is either min() or max(),
>> * the argument to the aggregate function is a column value,
>> * the type of column is not SCALAR.
>> 
>> And the changes above disables this optimisation if column type is SCALAR.
>> I also added test for this case.
> 
> This optimisation disables index search for min/max queries and SCALAR.
> I’m not sure that it is what we want. Nevertheless it makes usage of min/max
> uniform (in both cases error is raised), it dramatically slows down execution.
> I’ve asked server team, and we decided to allow all comparisons with SCALAR
> field type. Now we can’t tell scalar from non-scalar fields. To achieve this we should
> add field_type member to struct Mem and check field_type before comparisons.
> 
> Theoretically speaking, these checks introduced in minmaxStep still may occur
> useful: if user register function, which may return data of different types and use
> this function as argument of min/max functions.
> 
> Should current patch be pushed before handling of scalars is introduced - IDK.
> 
> Overall, I suggest following diff. Note that you don’t need even
> ‘reference_type’ in minmax context.
> 
> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
> index 0738f44e6..cc63f8b5f 100644
> --- a/src/box/sql/func.c
> +++ b/src/box/sql/func.c
> @@ -51,13 +51,7 @@
>  */
> struct minmax_context {
>        /** Value being aggregated i.e. current MAX or MIN. */
> -       Mem best;
> -       /**
> -        * Reference type to keep track of previous argument's types.
> -        * One of MEM_Null, MEM_Str, MEM_Int, MEM_Real, MEM_Blob
> -        * or MEM_Bool.
> -        */
> -       int reference_type;
> +       struct Mem best;
> };
> 
> static UConverter* pUtf8conv;
> @@ -1618,12 +1612,10 @@ minmaxStep(sql_context *context, int not_used, sql_value **argv)
>                return;
> 
>        sql_value *arg = argv[0];
> -       sql_value *best = &(minmax_context->best);
> -       if (best == NULL)
> -               return;
> +       sql_value *best = &minmax_context->best;
> 
> -       int sql_type = mem_type(arg);
> -       if (sql_type == MEM_Null) {
> +       int mem_type_current = arg->flags & MEM_PURE_TYPE_MASK;
> +       if (mem_type_current == MEM_Null) {
>                if (best->flags != 0)
>                        sqlSkipAccumulatorLoad(context);
>        } else if (best->flags != 0) {
> @@ -1636,25 +1628,20 @@ minmaxStep(sql_context *context, int not_used, sql_value **argv)
>                 * any other type). In the later case an error
>                 * is raised.
>                 */
> -               if (minmax_context->reference_type == 0)
> -                       minmax_context->reference_type = sql_type;
> -               int ref_sql_type = minmax_context->reference_type;
> -
> -               if (sql_type != ref_sql_type) {
> -                       bool types_are_compatible = (sql_type & MEM_NumMask) &&
> -                                                   (ref_sql_type & MEM_NumMask);
> +               int mem_type_best = best->flags & MEM_PURE_TYPE_MASK;
> +               if (mem_type_best != mem_type_current) {
> +                       bool types_are_compatible = (mem_type_best & MEM_NumMask) &&
> +                                                   (mem_type_current & MEM_NumMask);
>                        if (!types_are_compatible) {
>                                diag_set(ClientError, ER_INCONSISTENT_TYPES,
> -                                        type_to_str(ref_sql_type),
> -                                        type_to_str(sql_type));
> +                                        mem_type_to_str(best),
> +                                        mem_type_to_str(arg));
>                                context->fErrorOrAux = 1;
>                                context->isError = SQL_TARANTOOL_ERROR;
>                                return;
>                        }
>                }
> 
> -               int max;
> -               int cmp;
>                struct coll *coll = sqlGetFuncCollSeq(context);
>                /* This step function is used for both the min() and max() aggregates,
>                 * the only difference between the two being that the sense of the
> @@ -1664,9 +1651,9 @@ minmaxStep(sql_context *context, int not_used, sql_value **argv)
>                 * Therefore the next statement sets variable 'max' to 1 for the max()
>                 * aggregate, or 0 for min().
>                 */
> -               max = sql_user_data(context) != 0;
> -               cmp = sqlMemCompare(best, arg, coll);
> -               if ((max != 0 && cmp < 0) || (max == 0 && cmp > 0)) {
> +               bool max = sql_user_data(context) != 0;
> +               int cmp_res = sqlMemCompare(best, arg, coll);
> +               if ((max && cmp_res < 0) || (! max && cmp_res > 0)) {
>                        sqlVdbeMemCopy(best, arg);
>                } else {
>                        sqlSkipAccumulatorLoad(context);
> @@ -1682,12 +1669,13 @@ minMaxFinalize(sql_context * context)
> {
>        struct minmax_context *minmax_context = (struct minmax_context *)
>                sql_aggregate_context(context, sizeof(*minmax_context));
> -       sql_value *res = &(minmax_context->best);
> +       sql_value *res = &minmax_context->best;
> 
>        if (res != NULL) {
>                if (res->flags != 0) {
>                        sql_result_value(context, res);
>                }
> +               sqlVdbeMemRelease(res);
>        }
> }
> 
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index dd87c5b70..b1ec8c758 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -4340,22 +4340,13 @@ pushDownWhereTerms(Parse * pParse,      /* Parse context (for malloc() and error repo
>  * argument, this function checks if the following are true:
>  *
>  *    * the query contains just a single aggregate function,
> - *    * the aggregate function is either min() or max(),
> - *    * the argument to the aggregate function is a column value,
> - *    * the type of column is not SCALAR.
> + *    * the aggregate function is either min() or max(), and
> + *    * the argument to the aggregate function is a column value.
>  *
>  * If all of the above are true, then WHERE_ORDERBY_MIN or WHERE_ORDERBY_MAX
>  * is returned as appropriate. Also, *ppMinMax is set to point to the
>  * list of arguments passed to the aggregate before returning.
>  *
> - * The requirement of column type not being SCALAR follows from
> - * the purpose of the function. The purpose of the function is
> - * to answer the question: "Should MIN/MAX call be optimised by
> - * using ORDER BY clause code?" If the type of column is SCALAR
> - * then we have to iterate over all rows to check if their types
> - * are compatible. Hence, the optimisation should not be used.
> - * For details please see: https://github.com/tarantool/tarantool/issues/4032
> - *
>  * Or, if the conditions above are not met, *ppMinMax is set to 0 and
>  * WHERE_ORDERBY_NORMAL is returned.
>  */
> @@ -4373,8 +4364,6 @@ minMaxQuery(AggInfo * pAggInfo, ExprList ** ppMinMax)
>                if (pEList && pEList->nExpr == 1
>                    && pEList->a[0].pExpr->op == TK_AGG_COLUMN) {
>                        const char *zFunc = pExpr->u.zToken;
> -                       if (sql_expr_type(pEList->a[0].pExpr) == FIELD_TYPE_SCALAR)
> -                               return eRet;
>                        if (sqlStrICmp(zFunc, "min") == 0) {
>                                eRet = WHERE_ORDERBY_MIN;
>                                *ppMinMax = pEList;
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 960030b52..ed7bf8870 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -618,9 +618,10 @@ vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
> }
> 
> char *
> -type_to_str(int type)
> +mem_type_to_str(const struct Mem *p)
> {
> -       switch (type) {
> +       assert(p != NULL);
> +       switch (p->flags & MEM_PURE_TYPE_MASK) {
>        case MEM_Null:
>                return "NULL";
>        case MEM_Str:
> @@ -638,19 +639,6 @@ type_to_str(int type)
>        }
> }
> 
> -int
> -mem_type(const struct Mem *p)
> -{
> -       assert(p != NULL);
> -       return p->flags & MEM_PURE_TYPE_MASK;
> -}
> -
> -char *
> -mem_type_to_str(const struct Mem *p)
> -{
> -       return type_to_str(mem_type(p));
> -}
> -
> /*
>  * Execute as much of a VDBE program as we can.
>  * This is the core of sql_step().
> diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
> index 553c4f225..2b60bd6b5 100644
> --- a/src/box/sql/vdbeInt.h
> +++ b/src/box/sql/vdbeInt.h
> @@ -263,25 +263,11 @@ enum {
>        MEM_PURE_TYPE_MASK = 0x1f
> };
> 
> -/**
> - * Returns one of MEM_Null, MEM_Str, MEM_Int, MEM_Real, MEM_Blob
> - * or MEM_Bool. Used for error detection and reporting.
> - */
> -int
> -mem_type(const struct Mem *p);
> -
> /**
>  * Simple type to str convertor. It is used to simplify
>  * error reporting.
>  */
> char *
> -type_to_str(int type);
> -
> -/**
> - * Returns string representing type of the given Mem. It is used
> - * to simplify error reporting.
> - */
> -char *
> mem_type_to_str(const struct Mem *p);
Thank you very much, I have checked the diff and applied it with little changes.
Full diff:

---
 src/box/sql/func.c            | 62 ++++++++++++++++++++---------------
 src/box/sql/select.c          | 15 ++-------
 src/box/sql/sqlInt.h          |  1 +
 src/box/sql/vdbe.c            | 18 ++--------
 src/box/sql/vdbeInt.h         | 15 ---------
 test/sql-tap/minmax4.test.lua | 13 +++++---
 6 files changed, 49 insertions(+), 75 deletions(-)

diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index 0738f44e6..d98c60914 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -51,13 +51,7 @@
  */
 struct minmax_context {
 	/** Value being aggregated i.e. current MAX or MIN. */
-	Mem best;
-	/**
-	 * Reference type to keep track of previous argument's types.
-	 * One of MEM_Null, MEM_Str, MEM_Int, MEM_Real, MEM_Blob
-	 * or MEM_Bool.
-	 */
-	int reference_type;
+	struct Mem best;
 };
 
 static UConverter* pUtf8conv;
@@ -1605,6 +1599,25 @@ countFinalize(sql_context * context)
 	sql_result_int64(context, p ? p->n : 0);
 }
 
+/**
+ * Table of aggregate functions args type compatibility.
+ */
+static const bool scalar_type_compatibility[] = {
+	      /*  SQL_INTEGER  SQL_FLOAT  SQL_TEXT  SQL_BLOB  SQL_NULL */
+/* SQL_INTEGER */ true,        true,      false,    false,    false,
+/* SQL_FLOAT   */ true,        true,      false,    false,    false,
+/* SQL_TEXT    */ false,       false,     true,     false,    false,
+/* SQL_BLOB    */ false,       false,     false,    true,     false,
+/* SQL_NULL    */ false,       false,     false,    false,    true,
+};
+
+static bool
+are_scalar_types_compatible(enum sql_type type1, enum sql_type type2)
+{
+	int idx = (type2 - 1) * (SQL_TYPE_MAX - 1) + (type1 - 1);
+	return scalar_type_compatibility[idx];
+}
+
 /*
  * Routines to implement min() and max() aggregate functions.
  */
@@ -1618,12 +1631,10 @@ minmaxStep(sql_context *context, int not_used, sql_value **argv)
 		return;
 
 	sql_value *arg = argv[0];
-	sql_value *best = &(minmax_context->best);
-	if (best == NULL)
-		return;
+	sql_value *best = &minmax_context->best;
 
-	int sql_type = mem_type(arg);
-	if (sql_type == MEM_Null) {
+	enum sql_type sql_type_current = sql_value_type(arg);
+	if (sql_type_current == SQL_NULL) {
 		if (best->flags != 0)
 			sqlSkipAccumulatorLoad(context);
 	} else if (best->flags != 0) {
@@ -1636,25 +1647,21 @@ minmaxStep(sql_context *context, int not_used, sql_value **argv)
 		 * any other type). In the later case an error
 		 * is raised.
 		 */
-		if (minmax_context->reference_type == 0)
-			minmax_context->reference_type = sql_type;
-		int ref_sql_type = minmax_context->reference_type;
-
-		if (sql_type != ref_sql_type) {
-			bool types_are_compatible = (sql_type & MEM_NumMask) &&
-						    (ref_sql_type & MEM_NumMask);
+		enum sql_type sql_type_best = sql_value_type(best);
+		if (sql_type_best != sql_type_current) {
+			bool types_are_compatible =
+				are_scalar_types_compatible(sql_type_best,
+							    sql_type_current);
 			if (!types_are_compatible) {
 				diag_set(ClientError, ER_INCONSISTENT_TYPES,
-					 type_to_str(ref_sql_type),
-					 type_to_str(sql_type));
+					 mem_type_to_str(best),
+					 mem_type_to_str(arg));
 				context->fErrorOrAux = 1;
 				context->isError = SQL_TARANTOOL_ERROR;
 				return;
 			}
 		}
 
-		int max;
-		int cmp;
 		struct coll *coll = sqlGetFuncCollSeq(context);
 		/* This step function is used for both the min() and max() aggregates,
 		 * the only difference between the two being that the sense of the
@@ -1664,9 +1671,9 @@ minmaxStep(sql_context *context, int not_used, sql_value **argv)
 		 * Therefore the next statement sets variable 'max' to 1 for the max()
 		 * aggregate, or 0 for min().
 		 */
-		max = sql_user_data(context) != 0;
-		cmp = sqlMemCompare(best, arg, coll);
-		if ((max != 0 && cmp < 0) || (max == 0 && cmp > 0)) {
+		bool max = sql_user_data(context) != 0;
+		int cmp_res = sqlMemCompare(best, arg, coll);
+		if ((max && cmp_res < 0) || (! max && cmp_res > 0)) {
 			sqlVdbeMemCopy(best, arg);
 		} else {
 			sqlSkipAccumulatorLoad(context);
@@ -1682,12 +1689,13 @@ minMaxFinalize(sql_context * context)
 {
 	struct minmax_context *minmax_context = (struct minmax_context *)
 		sql_aggregate_context(context, sizeof(*minmax_context));
-	sql_value *res = &(minmax_context->best);
+	sql_value *res = &minmax_context->best;
 
 	if (res != NULL) {
 		if (res->flags != 0) {
 			sql_result_value(context, res);
 		}
+		sqlVdbeMemRelease(res);
 	}
 }
 
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index dd87c5b70..b1ec8c758 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -4340,22 +4340,13 @@ pushDownWhereTerms(Parse * pParse,	/* Parse context (for malloc() and error repo
  * argument, this function checks if the following are true:
  *
  *    * the query contains just a single aggregate function,
- *    * the aggregate function is either min() or max(),
- *    * the argument to the aggregate function is a column value,
- *    * the type of column is not SCALAR.
+ *    * the aggregate function is either min() or max(), and
+ *    * the argument to the aggregate function is a column value.
  *
  * If all of the above are true, then WHERE_ORDERBY_MIN or WHERE_ORDERBY_MAX
  * is returned as appropriate. Also, *ppMinMax is set to point to the
  * list of arguments passed to the aggregate before returning.
  *
- * The requirement of column type not being SCALAR follows from
- * the purpose of the function. The purpose of the function is
- * to answer the question: "Should MIN/MAX call be optimised by
- * using ORDER BY clause code?" If the type of column is SCALAR
- * then we have to iterate over all rows to check if their types
- * are compatible. Hence, the optimisation should not be used.
- * For details please see: https://github.com/tarantool/tarantool/issues/4032
- *
  * Or, if the conditions above are not met, *ppMinMax is set to 0 and
  * WHERE_ORDERBY_NORMAL is returned.
  */
@@ -4373,8 +4364,6 @@ minMaxQuery(AggInfo * pAggInfo, ExprList ** ppMinMax)
 		if (pEList && pEList->nExpr == 1
 		    && pEList->a[0].pExpr->op == TK_AGG_COLUMN) {
 			const char *zFunc = pExpr->u.zToken;
-			if (sql_expr_type(pEList->a[0].pExpr) == FIELD_TYPE_SCALAR)
-				return eRet;
 			if (sqlStrICmp(zFunc, "min") == 0) {
 				eRet = WHERE_ORDERBY_MIN;
 				*ppMinMax = pEList;
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index b322602dc..a96033db5 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -641,6 +641,7 @@ enum sql_type {
 	SQL_TEXT = 3,
 	SQL_BLOB = 4,
 	SQL_NULL = 5,
+	SQL_TYPE_MAX = 6,
 };
 
 /**
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 960030b52..ed7bf8870 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -618,9 +618,10 @@ vdbe_add_new_autoinc_id(struct Vdbe *vdbe, int64_t id)
 }
 
 char *
-type_to_str(int type)
+mem_type_to_str(const struct Mem *p)
 {
-	switch (type) {
+	assert(p != NULL);
+	switch (p->flags & MEM_PURE_TYPE_MASK) {
 	case MEM_Null:
 		return "NULL";
 	case MEM_Str:
@@ -638,19 +639,6 @@ type_to_str(int type)
 	}
 }
 
-int
-mem_type(const struct Mem *p)
-{
-	assert(p != NULL);
-	return p->flags & MEM_PURE_TYPE_MASK;
-}
-
-char *
-mem_type_to_str(const struct Mem *p)
-{
-	return type_to_str(mem_type(p));
-}
-
 /*
  * Execute as much of a VDBE program as we can.
  * This is the core of sql_step().
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 553c4f225..c84f22caf 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -228,7 +228,6 @@ struct Mem {
 #define MEM_Str       0x0002	/* Value is a string */
 #define MEM_Int       0x0004	/* Value is an integer */
 #define MEM_Real      0x0008	/* Value is a real number */
-#define MEM_NumMask   0x000c	/* Value is integer or real (mask) */
 #define MEM_Blob      0x0010	/* Value is a BLOB */
 #define MEM_Bool      0x0020    /* Value is a bool */
 #define MEM_Ptr       0x0040	/* Value is a generic pointer */
@@ -263,25 +262,11 @@ enum {
 	MEM_PURE_TYPE_MASK = 0x1f
 };
 
-/**
- * Returns one of MEM_Null, MEM_Str, MEM_Int, MEM_Real, MEM_Blob
- * or MEM_Bool. Used for error detection and reporting.
- */
-int
-mem_type(const struct Mem *p);
-
 /**
  * Simple type to str convertor. It is used to simplify
  * error reporting.
  */
 char *
-type_to_str(int type);
-
-/**
- * Returns string representing type of the given Mem. It is used
- * to simplify error reporting.
- */
-char *
 mem_type_to_str(const struct Mem *p);
 
 /* Return TRUE if Mem X contains dynamically allocated content - anything
diff --git a/test/sql-tap/minmax4.test.lua b/test/sql-tap/minmax4.test.lua
index 7476051f1..7e6e309ad 100755
--- a/test/sql-tap/minmax4.test.lua
+++ b/test/sql-tap/minmax4.test.lua
@@ -353,13 +353,16 @@ test:do_test(
             SELECT MAX(b) FROM t4;
         ]]
     end, {
-        1, "Inconsistent types: expected REAL got TEXT"
+        1, "Inconsistent types: expected INTEGER got TEXT"
     })
 
 -- Cases when we call aggregate MIN/MAX functions on column with
 -- index (e.g. PRIMARY KEY index) deserves it's own test
 -- because in this case MIN/MAX is implemented not with
 -- dedicated function, but with usage of corresponding index.
+-- The behavior is different: in such cases MIN/MAX are less
+-- type-strict, for example it's possible to compare numeri
+-- values with text values.
 test:do_test(
     "minmax4-3.5",
     function()
@@ -386,22 +389,22 @@ test:do_test(
 test:do_test(
     "minmax4-3.7",
     function()
-        return test:catchsql [[
+        return test:execsql [[
             INSERT INTO t5 VALUES ('abc');
             SELECT MIN(a) FROM t5;
         ]]
     end, {
-        1, "Inconsistent types: expected INTEGER got TEXT"
+        1.5
     })
 
 test:do_test(
     "minmax4-3.8",
     function()
-        return test:catchsql [[
+        return test:execsql [[
             SELECT MAX(a) FROM t5;
         ]]
     end, {
-        1, "Inconsistent types: expected INTEGER got TEXT"
+        'abc'
     })
 
 test:finish_test()
-- 
2.20.1