[tarantool-patches] [PATCH v2 0/2] sql: add ARRAY, MAP and ANY types to mem_apply_type()

[tarantool-patches] [PATCH v2 0/2] sql: add ARRAY, MAP and ANY types to mem_apply_type()

[imeevma]

This patch-set allows you to use the UPDATE statement to modify
rows in space created in Lua. Previously, this was not possible if
the space had MAP, ARRAY or ANY fields in its format.

https://github.com/tarantool/tarantool/issues/4189
https://github.com/tarantool/tarantool/tree/imeevma/gh-4189-field-type-conversion-error

Mergen Imeev (2):
  sql: add ARRAY, MAP and ANY types to mem_apply_type()
  sql: fix error in case ARRAY/MAP converted to SCALAR

 src/box/sql/vdbe.c      |  48 +++++++++++++++++++++--
 test/sql/types.result   | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/types.test.lua |  37 ++++++++++++++++++
 3 files changed, 184 insertions(+), 3 deletions(-)

-- 
2.7.4

[tarantool-patches] [PATCH v2 1/2] sql: add ARRAY, MAP and ANY types to mem_apply_type()

[imeevma]

Function mem_apply_type() implements implicit type conversion. As
a rule, tuple to be inserted to the space is exposed to this
conversion which is invoked during execution of OP_MakeRecord
opcode (which in turn forms tuple). This function was not adjusted
to operate on ARRAY, MAP and ANY field types since they are poorly
supported in current SQL implementation. Hence, when tuple to be
inserted in space having mentioned field types reaches this
function, it results in error. Note that we can't set ARRAY or MAP
types in SQL, but such situation may appear during UPDATE
operation on space created via Lua interface. This problem is
solved by extending implicit type conversions with obvious casts:
array field can be casted to array, map to map and any to any.

Closes #4189
---
 src/box/sql/vdbe.c      | 35 ++++++++++++++++++++++++++++++++++-
 test/sql/types.result   | 40 ++++++++++++++++++++++++++++++++++++++++
 test/sql/types.test.lua | 15 +++++++++++++++
 3 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6a4a303..1200ff4 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -292,7 +292,19 @@ mem_apply_numeric_type(struct Mem *record)
  *    Convert mem to a string representation.
  *
  * SCALAR:
- *    Mem is unchanged, but flag is set to BLOB.
+ *    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.
@@ -338,6 +350,27 @@ mem_apply_type(struct Mem *record, enum field_type type)
 		record->flags &= ~(MEM_Real | MEM_Int);
 		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;
diff --git a/test/sql/types.result b/test/sql/types.result
index 5abe6e0..0dba69f 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1110,6 +1110,46 @@ box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
     type: unsigned
   rows: []
 ...
+--
+-- gh-4189: make sure that update doesn't throw an error if format
+-- of table features map/array field types.
+--
+format = {}
+---
+...
+format[1] = {type = 'integer', name = 'I'}
+---
+...
+format[2] = {type = 'boolean', name = 'B'}
+---
+...
+format[3] = {type = 'array', name = 'F1'}
+---
+...
+format[4] = {type = 'map', name = 'F2'}
+---
+...
+format[5] = {type = 'any', name = 'F3'}
+---
+...
+s = box.schema.space.create('T', {format = format})
+---
+...
+ii = s:create_index('ii')
+---
+...
+s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
+---
+- [1, true, [1, 2], {'a': 3}, 'asd']
+...
+box.execute('UPDATE t SET b = false WHERE i = 1;')
+---
+- row_count: 1
+...
+s:select()
+---
+- - [1, false, [1, 2], {'a': 3}, 'asd']
+...
 s:drop()
 ---
 ...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 410864a..22cb105 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -275,4 +275,19 @@ _ = s:create_index('sk', { parts = { 'A' } })
 s:insert({ 1, 1 })
 box.execute("SELECT a FROM t1 WHERE a IN (1.1, 2.1);")
 
+--
+-- gh-4189: make sure that update doesn't throw an error if format
+-- of table features map/array field types.
+--
+format = {}
+format[1] = {type = 'integer', name = 'I'}
+format[2] = {type = 'boolean', name = 'B'}
+format[3] = {type = 'array', name = 'F1'}
+format[4] = {type = 'map', name = 'F2'}
+format[5] = {type = 'any', name = 'F3'}
+s = box.schema.space.create('T', {format = format})
+ii = s:create_index('ii')
+s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
+box.execute('UPDATE t SET b = false WHERE i = 1;')
+s:select()
 s:drop()
-- 
2.7.4

[tarantool-patches] [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[imeevma]

Since ARRAY and MAP cannot be converted to a scalar, this
operation should throw an error. But when the error was throws
from SQL, the error was unreadable. The reason for this is that
the given array or map was not correctly converted to a string.
This patch fixes the problem by printing an 'array' or 'map' in
case of an error due to a conversion from ARRAY or MAP,
respectively.
For example:

box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
format = {}
format[1] = {type = 'integer', name = 'I'}
format[2] = {type = 'array', name = 'A'}
s = box.schema.space.create('T2', {format=format})
i = s:create_index('ii')
s:insert({1, {1,2,3}})
box.execute('INSERT INTO t1 SELECT * FROM t2;')

Should return:
- error: 'Type mismatch: can not convert array to scalar'

Follow-up #4189
---
 src/box/sql/vdbe.c      | 13 +++++++++--
 test/sql/types.result   | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
 test/sql/types.test.lua | 22 ++++++++++++++++++
 3 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 1200ff4..f6aaaa6 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2704,8 +2704,17 @@ case OP_ApplyType: {
 		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
 		assert(memIsValid(pIn1));
 		if (mem_apply_type(pIn1, type) != 0) {
-			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
-				 sql_value_text(pIn1),
+			const char *value;
+			if ((pIn1->flags & MEM_Subtype) != 0 &&
+			    pIn1->subtype == SQL_SUBTYPE_MSGPACK) {
+				if (mp_typeof(*pIn1->z) == MP_MAP)
+					value = "map";
+				else
+					value = "array";
+			} else {
+				value = (const char *)sql_value_text(pIn1);
+			}
+			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, value,
 				 field_type_strs[type]);
 			goto abort_due_to_error;
 		}
diff --git a/test/sql/types.result b/test/sql/types.result
index 0dba69f..61a760f 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1153,3 +1153,65 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('DROP TABLE IF EXISTS t1;')
+---
+- row_count: 1
+...
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+---
+- row_count: 1
+...
+format = {}
+---
+...
+format[1] = {type = 'integer', name = 'I'}
+---
+...
+format[2] = {type = 'array', name = 'A'}
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- error: 'Type mismatch: can not convert array to scalar'
+...
+s:drop()
+---
+...
+format[2].type = 'map'
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {b = 1}})
+---
+- [1, {'b': 1}]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- error: 'Type mismatch: can not convert map to scalar'
+...
+s:drop()
+---
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 22cb105..a99cc9f 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -291,3 +291,25 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
 box.execute('UPDATE t SET b = false WHERE i = 1;')
 s:select()
 s:drop()
+
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('DROP TABLE IF EXISTS t1;')
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+format = {}
+format[1] = {type = 'integer', name = 'I'}
+format[2] = {type = 'array', name = 'A'}
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {1,2,3}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+format[2].type = 'map'
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {b = 1}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+box.execute('DROP TABLE t1;')
-- 
2.7.4

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[n.pettik]

> ---
> src/box/sql/vdbe.c      | 13 +++++++++--
> test/sql/types.result   | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
> test/sql/types.test.lua | 22 ++++++++++++++++++
> 3 files changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 1200ff4..f6aaaa6 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2704,8 +2704,17 @@ case OP_ApplyType: {
> 		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> 		assert(memIsValid(pIn1));
> 		if (mem_apply_type(pIn1, type) != 0) {
> -			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> -				 sql_value_text(pIn1),
> +			const char *value;
> +			if ((pIn1->flags & MEM_Subtype) != 0 &&
> +			    pIn1->subtype == SQL_SUBTYPE_MSGPACK) {
> +				if (mp_typeof(*pIn1->z) == MP_MAP)
> +					value = "map";
> +				else
> +					value = "array";
> +			} else {
> +				value = (const char *)sql_value_text(pIn1);


Why not simply patch sql_value_text() to make it convert
map/array to string representation? I’m afraid this is
unlikely to be the only place where such error may occur.

> +			}
> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, value,
> 				 field_type_strs[type]);
> 			goto abort_due_to_error;
> 		}
> diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> index 22cb105..a99cc9f 100644
> --- a/test/sql/types.test.lua
> +++ b/test/sql/types.test.lua
> @@ -291,3 +291,25 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
> box.execute('UPDATE t SET b = false WHERE i = 1;')
> s:select()
> s:drop()
> +
> +--
> +-- Make sure that the array/map conversion to scalar error is
> +-- displayed correctly.
> +--
> +box.execute('DROP TABLE IF EXISTS t1;’)

Why ‘IF EXISTS’? Btw I’d better use space:drop() to avoid
SQL’s overhead.

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/07/24 17:03]:
> > 		assert(memIsValid(pIn1));
> > 		if (mem_apply_type(pIn1, type) != 0) {
> > -			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > -				 sql_value_text(pIn1),
> > +			const char *value;
> > +			if ((pIn1->flags & MEM_Subtype) != 0 &&
> > +			    pIn1->subtype == SQL_SUBTYPE_MSGPACK) {
> > +				if (mp_typeof(*pIn1->z) == MP_MAP)
> > +					value = "map";
> > +				else
> > +					value = "array";
> > +			} else {
> > +				value = (const char *)sql_value_text(pIn1);
> 
> 
> Why not simply patch sql_value_text() to make it convert
> map/array to string representation? I’m afraid this is
> unlikely to be the only place where such error may occur.

Perhaps I am missing the context, but because we don't want to 
implicitly convert these values to text in SQL? Soon we will be
able to work with these values in SQL queries, so we simply need
to make sure we can pass them around expression trees for now?

-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[n.pettik]

> On 25 Jul 2019, at 01:37, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * n.pettik <korablev@tarantool.org> [19/07/24 17:03]:
>>> 		assert(memIsValid(pIn1));
>>> 		if (mem_apply_type(pIn1, type) != 0) {
>>> -			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
>>> -				 sql_value_text(pIn1),
>>> +			const char *value;
>>> +			if ((pIn1->flags & MEM_Subtype) != 0 &&
>>> +			    pIn1->subtype == SQL_SUBTYPE_MSGPACK) {
>>> +				if (mp_typeof(*pIn1->z) == MP_MAP)
>>> +					value = "map";
>>> +				else
>>> +					value = "array";
>>> +			} else {
>>> +				value = (const char *)sql_value_text(pIn1);
>> 
>> 
>> Why not simply patch sql_value_text() to make it convert
>> map/array to string representation? I’m afraid this is
>> unlikely to be the only place where such error may occur.
> 
> Perhaps I am missing the context, but because we don't want to 
> implicitly convert these values to text in SQL?

It’s not about implicit conversion, it’s only about text representation
(like mp_fprint()).

> Soon we will be able to work with these values in SQL queries,

It’s extremely doubtful statement.

> so we simply need
> to make sure we can pass them around expression trees for now?

Actually, we can’t now.

> 
> -- 
> Konstantin Osipov, Moscow, Russia
> 

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Mergen Imeev]

Hi! Thank you for review. My answers and new patch below.
I didn't include diff since patch is quite obvious by
itself.

On Wed, Jul 24, 2019 at 03:24:32PM +0300, n.pettik wrote:
> 
> > ---
> > src/box/sql/vdbe.c      | 13 +++++++++--
> > test/sql/types.result   | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
> > test/sql/types.test.lua | 22 ++++++++++++++++++
> > 3 files changed, 95 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 1200ff4..f6aaaa6 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -2704,8 +2704,17 @@ case OP_ApplyType: {
> > 		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> > 		assert(memIsValid(pIn1));
> > 		if (mem_apply_type(pIn1, type) != 0) {
> > -			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > -				 sql_value_text(pIn1),
> > +			const char *value;
> > +			if ((pIn1->flags & MEM_Subtype) != 0 &&
> > +			    pIn1->subtype == SQL_SUBTYPE_MSGPACK) {
> > +				if (mp_typeof(*pIn1->z) == MP_MAP)
> > +					value = "map";
> > +				else
> > +					value = "array";
> > +			} else {
> > +				value = (const char *)sql_value_text(pIn1);
> 
> 
> Why not simply patch sql_value_text() to make it convert
> map/array to string representation? I’m afraid this is
> unlikely to be the only place where such error may occur.
> 
Done. I used region to create the result that is returned
by the sql_value_text() function in case the argument was
a BLOB with msgpack of type array or map.

> > +			}
> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, value,
> > 				 field_type_strs[type]);
> > 			goto abort_due_to_error;
> > 		}
> > diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> > index 22cb105..a99cc9f 100644
> > --- a/test/sql/types.test.lua
> > +++ b/test/sql/types.test.lua
> > @@ -291,3 +291,25 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
> > box.execute('UPDATE t SET b = false WHERE i = 1;')
> > s:select()
> > s:drop()
> > +
> > +--
> > +-- Make sure that the array/map conversion to scalar error is
> > +-- displayed correctly.
> > +--
> > +box.execute('DROP TABLE IF EXISTS t1;’)
> 
> Why ‘IF EXISTS’? Btw I’d better use space:drop() to avoid
> SQL’s overhead.
> 
> 
I dropped this line since it was fixed after rebase.

New patch:

From 5dde6f6398ca9845c3a17173b9e9625bc1b20d32 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 22 Jul 2019 12:54:34 +0300
Subject: [PATCH] sql: fix error in case ARRAY/MAP converted to SCALAR

Since ARRAY and MAP cannot be converted to a scalar, this
operation should throw an error. But when the error was throws
from SQL, the error was unreadable. The reason for this is that
the given array or map was not correctly converted to a string.
This patch fixes the problem by converting ARRAY or MAP to their
string representation.
For example:

box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
format = {}
format[1] = {type = 'integer', name = 'I'}
format[2] = {type = 'array', name = 'A'}
s = box.schema.space.create('T2', {format=format})
i = s:create_index('ii')
s:insert({1, {1,2,3}})
box.execute('INSERT INTO t1 SELECT * FROM t2;')

Should return:
- error: 'Type mismatch: can not convert [1, 2, 3] to scalar'

Follow-up #4189

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 847a6b0..8bea46b 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1135,6 +1135,19 @@ valueToText(sql_value * pVal)
 {
 	assert(pVal != 0);
 	assert((pVal->flags & (MEM_Null)) == 0);
+	if ((pVal->flags & MEM_Subtype) != 0 &&
+	    pVal->subtype == SQL_SUBTYPE_MSGPACK) {
+		const char *value = mp_str(pVal->z);
+		size_t len = strlen(value) + 1;
+		char *result = region_alloc(&fiber()->gc, len);
+		if (result == NULL) {
+			diag_set(OutOfMemory, len, "region_alloc", "result");
+			sqlOomFault(sql_get());
+			return NULL;
+		}
+		memcpy(result, value, len);
+		return result;
+	}
 	if (pVal->flags & (MEM_Blob | MEM_Str)) {
 		if (ExpandBlob(pVal))
 			return 0;
diff --git a/test/sql/types.result b/test/sql/types.result
index 15e359d..95cbd7a 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1794,3 +1794,61 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+---
+- row_count: 1
+...
+format = {}
+---
+...
+format[1] = {type = 'integer', name = 'I'}
+---
+...
+format[2] = {type = 'array', name = 'A'}
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
+...
+s:drop()
+---
+...
+format[2].type = 'map'
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {b = 1}})
+---
+- [1, {'b': 1}]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- error: 'Type mismatch: can not convert {"b": 1} to scalar'
+...
+s:drop()
+---
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index de1b7ac..190bdd2 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -416,3 +416,24 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
 box.execute('UPDATE t SET b = false WHERE i = 1;')
 s:select()
 s:drop()
+
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+format = {}
+format[1] = {type = 'integer', name = 'I'}
+format[2] = {type = 'array', name = 'A'}
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {1,2,3}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+format[2].type = 'map'
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {b = 1}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+box.execute('DROP TABLE t1;')

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[n.pettik]

> 
> New patch:
> 
> From 5dde6f6398ca9845c3a17173b9e9625bc1b20d32 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Mon, 22 Jul 2019 12:54:34 +0300
> Subject: [PATCH] sql: fix error in case ARRAY/MAP converted to SCALAR
> 
> Since ARRAY and MAP cannot be converted to a scalar, this
> operation should throw an error. But when the error was throws
> from SQL, the error was unreadable. The reason for this is that
> the given array or map was not correctly converted to a string.
> This patch fixes the problem by converting ARRAY or MAP to their
> string representation.
> For example:
> 
> box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
> format = {}
> format[1] = {type = 'integer', name = 'I'}
> format[2] = {type = 'array', name = 'A'}
> s = box.schema.space.create('T2', {format=format})
> i = s:create_index('ii')
> s:insert({1, {1,2,3}})
> box.execute('INSERT INTO t1 SELECT * FROM t2;')
> 
> Should return:
> - error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
> 
> Follow-up #4189

Fixed a bit commit message:

    sql: make valueToText() operate on MAP/ARRAY values
    
    Since ARRAY and MAP cannot be converted to SCALAR type, this operation
    should throw an error. But when the error is raised in SQL, it is
    displayed in unreadable form. The reason for this is that the given
    array or map is not correctly converted to a string. This patch fixes
    the problem by converting ARRAY or MAP to their string representation.
    For example:
    
    box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
    format = {}
    format[1] = {type = 'integer', name = 'I'}
    format[2] = {type = 'array', name = 'A'}
    s = box.schema.space.create('T2', {format=format})
    i = s:create_index('ii')
    s:insert({1, {1,2,3}})
    box.execute('INSERT INTO t1 SELECT * FROM t2;')
    
    Should return:
    - error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
    
    Follow-up #4189

> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index 847a6b0..8bea46b 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -1135,6 +1135,19 @@ valueToText(sql_value * pVal)
> {
> 	assert(pVal != 0);
> 	assert((pVal->flags & (MEM_Null)) == 0);
> +	if ((pVal->flags & MEM_Subtype) != 0 &&
> +	    pVal->subtype == SQL_SUBTYPE_MSGPACK) {
> +		const char *value = mp_str(pVal->z);
> +		size_t len = strlen(value) + 1;
> +		char *result = region_alloc(&fiber()->gc, len);
> +		if (result == NULL) {
> +			diag_set(OutOfMemory, len, "region_alloc", "result");
> +			sqlOomFault(sql_get());
> +			return NULL;
> +		}
> +		memcpy(result, value, len);
> +		return result;

That’s not what we need IMHO. Firstly, for all other memory types                  
valueToText() function returns pVal->z, i.e. this function firstly                 
converts value to string and then returns it.  Secondly, we don’t track            
value allocated on region: obviously if it is used after transaction               
commitment, it will lead to use-after-free bug. I can’t say whether this           
scenario is possible looking only on code.  You should rather patch                
sqlVdbeMemStringify: reserve memory using malloc, memcpy string to mem,            
change type of memory to MEM_Str. So that make it work with ARRAY and              
MAP types in the same way as with other types.                                     

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Mergen Imeev]

Hi! Thank you for review! My answers and new patch below.

On Wed, Aug 07, 2019 at 09:25:59PM +0300, n.pettik wrote:
> 
> > 
> > New patch:
> > 
> > From 5dde6f6398ca9845c3a17173b9e9625bc1b20d32 Mon Sep 17 00:00:00 2001
> > From: Mergen Imeev <imeevma@gmail.com>
> > Date: Mon, 22 Jul 2019 12:54:34 +0300
> > Subject: [PATCH] sql: fix error in case ARRAY/MAP converted to SCALAR
> > 
> > Since ARRAY and MAP cannot be converted to a scalar, this
> > operation should throw an error. But when the error was throws
> > from SQL, the error was unreadable. The reason for this is that
> > the given array or map was not correctly converted to a string.
> > This patch fixes the problem by converting ARRAY or MAP to their
> > string representation.
> > For example:
> > 
> > box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
> > format = {}
> > format[1] = {type = 'integer', name = 'I'}
> > format[2] = {type = 'array', name = 'A'}
> > s = box.schema.space.create('T2', {format=format})
> > i = s:create_index('ii')
> > s:insert({1, {1,2,3}})
> > box.execute('INSERT INTO t1 SELECT * FROM t2;')
> > 
> > Should return:
> > - error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
> > 
> > Follow-up #4189
> 
> Fixed a bit commit message:
> 
>     sql: make valueToText() operate on MAP/ARRAY values
>     
>     Since ARRAY and MAP cannot be converted to SCALAR type, this operation
>     should throw an error. But when the error is raised in SQL, it is
>     displayed in unreadable form. The reason for this is that the given
>     array or map is not correctly converted to a string. This patch fixes
>     the problem by converting ARRAY or MAP to their string representation.
>     For example:
>     
>     box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
>     format = {}
>     format[1] = {type = 'integer', name = 'I'}
>     format[2] = {type = 'array', name = 'A'}
>     s = box.schema.space.create('T2', {format=format})
>     i = s:create_index('ii')
>     s:insert({1, {1,2,3}})
>     box.execute('INSERT INTO t1 SELECT * FROM t2;')
>     
>     Should return:
>     - error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
>     
>     Follow-up #4189
> 
Thank you, fixed.

> > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > index 847a6b0..8bea46b 100644
> > --- a/src/box/sql/vdbemem.c
> > +++ b/src/box/sql/vdbemem.c
> > @@ -1135,6 +1135,19 @@ valueToText(sql_value * pVal)
> > {
> > 	assert(pVal != 0);
> > 	assert((pVal->flags & (MEM_Null)) == 0);
> > +	if ((pVal->flags & MEM_Subtype) != 0 &&
> > +	    pVal->subtype == SQL_SUBTYPE_MSGPACK) {
> > +		const char *value = mp_str(pVal->z);
> > +		size_t len = strlen(value) + 1;
> > +		char *result = region_alloc(&fiber()->gc, len);
> > +		if (result == NULL) {
> > +			diag_set(OutOfMemory, len, "region_alloc", "result");
> > +			sqlOomFault(sql_get());
> > +			return NULL;
> > +		}
> > +		memcpy(result, value, len);
> > +		return result;
> 
> That’s not what we need IMHO. Firstly, for all other memory types                  
> valueToText() function returns pVal->z, i.e. this function firstly                 
> converts value to string and then returns it.  Secondly, we don’t track            
> value allocated on region: obviously if it is used after transaction               
> commitment, it will lead to use-after-free bug. I can’t say whether this           
> scenario is possible looking only on code.  You should rather patch                
> sqlVdbeMemStringify: reserve memory using malloc, memcpy string to mem,            
> change type of memory to MEM_Str. So that make it work with ARRAY and              
> MAP types in the same way as with other types.                                     
> 
Fixed.

New patch:

From d0ffad654fe4ddf366bd4cfd4e1c9340a8044244 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 22 Jul 2019 12:54:34 +0300
Subject: [PATCH] sql: make valueToText() operate on MAP/ARRAY values

Since ARRAY and MAP cannot be converted to SCALAR type, this
operation should throw an error. But when the error is raised in
SQL, it is displayed in unreadable form. The reason for this is
that the given array or map is not correctly converted to a
string. This patch fixes the problem by converting ARRAY or MAP to
their string representation.
For example:

box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
format = {}
format[1] = {type = 'integer', name = 'I'}
format[2] = {type = 'array', name = 'A'}
s = box.schema.space.create('T2', {format=format})
i = s:create_index('ii')
s:insert({1, {1,2,3}})
box.execute('INSERT INTO t1 SELECT * FROM t2;')

Should return:
- error: 'Type mismatch: can not convert [1, 2, 3] to scalar'

Follow-up #4189

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 5516d7f..f94f118 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -281,15 +281,30 @@ int
 sqlVdbeMemStringify(Mem * pMem)
 {
 	int fg = pMem->flags;
-	const int nByte = 32;
+	int nByte = 32;
 
-	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
+	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
+	    ((pMem->flags & MEM_Subtype) == 0 ||
+	     pMem->subtype != SQL_SUBTYPE_MSGPACK))
 		return 0;
 
 	assert(!(fg & MEM_Zero));
-	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool)) != 0);
+	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool |
+		      MEM_Subtype)) != 0);
 	assert(EIGHT_BYTE_ALIGNMENT(pMem));
 
+	/*
+	 * In case we have ARRAY/MAP we should save decoded value
+	 * before clearing pMem->z.
+	 */
+	char *value = NULL;
+	if ((fg & MEM_Subtype) != 0 && pMem->subtype == SQL_SUBTYPE_MSGPACK) {
+		const char *value_str = mp_str(pMem->z);
+		nByte = strlen(value_str) + 1;
+		value = region_alloc(&fiber()->gc, nByte);
+		memcpy(value, value_str, nByte);
+	}
+
 	if (sqlVdbeMemClearAndResize(pMem, nByte)) {
 		return -1;
 	}
@@ -302,6 +317,11 @@ sqlVdbeMemStringify(Mem * pMem)
 	} else if ((fg & MEM_Bool) != 0) {
 		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
 		pMem->flags &= ~MEM_Bool;
+	} else if ((fg & MEM_Subtype) != 0 &&
+		   pMem->subtype == SQL_SUBTYPE_MSGPACK) {
+		sql_snprintf(nByte, pMem->z, "%s", value);
+		pMem->flags &= ~MEM_Subtype;
+		pMem->subtype = SQL_SUBTYPE_NO;
 	} else {
 		assert(fg & MEM_Real);
 		sql_snprintf(nByte, pMem->z, "%!.15g", pMem->u.r);
@@ -1148,7 +1168,9 @@ valueToText(sql_value * pVal)
 {
 	assert(pVal != 0);
 	assert((pVal->flags & (MEM_Null)) == 0);
-	if (pVal->flags & (MEM_Blob | MEM_Str)) {
+	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
+	    ((pVal->flags & MEM_Subtype) == 0 ||
+	     pVal->subtype != SQL_SUBTYPE_MSGPACK)) {
 		if (ExpandBlob(pVal))
 			return 0;
 		pVal->flags |= MEM_Str;
diff --git a/test/sql/types.result b/test/sql/types.result
index 1241ae4..6740c15 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -2266,3 +2266,74 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+---
+- row_count: 1
+...
+format = {}
+---
+...
+format[1] = {type = 'integer', name = 'I'}
+---
+...
+format[2] = {type = 'array', name = 'A'}
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert [1, 2, 3] to scalar'
+...
+s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
+---
+- [1, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,
+    22, 23, 24, 25, 26, 27, 28, 29, 30]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
+  15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30] to scalar'
+...
+s:drop()
+---
+...
+format[2].type = 'map'
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {b = 1}})
+---
+- [1, {'b': 1}]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert {"b": 1} to scalar'
+...
+s:drop()
+---
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index f6a2dfd..c9f0952 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -512,3 +512,26 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
 box.execute('UPDATE t SET b = false WHERE i = 1;')
 s:select()
 s:drop()
+
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+format = {}
+format[1] = {type = 'integer', name = 'I'}
+format[2] = {type = 'array', name = 'A'}
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {1,2,3}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+format[2].type = 'map'
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {b = 1}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+box.execute('DROP TABLE t1;')

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Nikita Pettik]

On Wed, Aug 28, 2019 at 04:30:02PM +0300, Mergen Imeev wrote:
> Hi! Thank you for review! My answers and new patch below.
> 
> On Wed, Aug 07, 2019 at 09:25:59PM +0300, n.pettik wrote:
> > 
> > > 
> 
> New patch:
> 
> From d0ffad654fe4ddf366bd4cfd4e1c9340a8044244 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Mon, 22 Jul 2019 12:54:34 +0300
> Subject: [PATCH] sql: make valueToText() operate on MAP/ARRAY values
> 
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index 5516d7f..f94f118 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -281,15 +281,30 @@ int
>  sqlVdbeMemStringify(Mem * pMem)
>  {
>  	int fg = pMem->flags;
> -	const int nByte = 32;
> +	int nByte = 32;
>  
> -	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
> +	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
> +	    ((pMem->flags & MEM_Subtype) == 0 ||
> +	     pMem->subtype != SQL_SUBTYPE_MSGPACK))
>  		return 0;

Why do you need OR condition? There's no any other subtype except for
SQL_SUBTYPE_MSGPACK. If the latter is set, MEM_Subtype must be set.

>  
> +		memcpy(value, value_str, nByte);
> +	}
> +
>  	if (sqlVdbeMemClearAndResize(pMem, nByte)) {
>  		return -1;
>  	}
> @@ -302,6 +317,11 @@ sqlVdbeMemStringify(Mem * pMem)
>  	} else if ((fg & MEM_Bool) != 0) {
>  		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
>  		pMem->flags &= ~MEM_Bool;
> +	} else if ((fg & MEM_Subtype) != 0 &&
> +		   pMem->subtype == SQL_SUBTYPE_MSGPACK) {
> +		sql_snprintf(nByte, pMem->z, "%s", value);
> +		pMem->flags &= ~MEM_Subtype;
> +		pMem->subtype = SQL_SUBTYPE_NO;
>  	} else {
>  		assert(fg & MEM_Real);
>  		sql_snprintf(nByte, pMem->z, "%!.15g", pMem->u.r);
> @@ -1148,7 +1168,9 @@ valueToText(sql_value * pVal)
>  {
>  	assert(pVal != 0);
>  	assert((pVal->flags & (MEM_Null)) == 0);
> -	if (pVal->flags & (MEM_Blob | MEM_Str)) {
> +	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
> +	    ((pVal->flags & MEM_Subtype) == 0 ||
> +	     pVal->subtype != SQL_SUBTYPE_MSGPACK)) {

You quite intensively abuse this check, so I'd better introduce
helper function for that purpose.

>  		if (ExpandBlob(pVal))
>  			return 0;
>  		pVal->flags |= MEM_Str;
> diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> index f6a2dfd..c9f0952 100644
> --- a/test/sql/types.test.lua
> +++ b/test/sql/types.test.lua
> @@ -512,3 +512,26 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
>  box.execute('UPDATE t SET b = false WHERE i = 1;')
>  s:select()
>  s:drop()
> +
> +--
> +-- Make sure that the array/map conversion to scalar error is
> +-- displayed correctly.
> +--
> +box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
> +format = {}
> +format[1] = {type = 'integer', name = 'I'}
> +format[2] = {type = 'array', name = 'A'}
> +s = box.schema.space.create('T2', {format=format})
> +i = s:create_index('ii')
> +s:insert({1, {1,2,3}})
> +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> +s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
> +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> +s:drop()
> +format[2].type = 'map'
> +s = box.schema.space.create('T2', {format=format})
> +i = s:create_index('ii')
> +s:insert({1, {b = 1}})
> +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> +s:drop()
> +box.execute('DROP TABLE t1;')

I'd also add synthetic test verifing that huge enough tuples (string
representation of which doesn't fit into static buffer) are processed
without accidents.

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Mergen Imeev]

Hi! Thank you for review. New patch and my answers below.
I didn't include diff between versions since patch is quite
obvious and all I changed is created the helper function.

On Thu, Aug 29, 2019 at 03:19:30PM +0300, Nikita Pettik wrote:
> On Wed, Aug 28, 2019 at 04:30:02PM +0300, Mergen Imeev wrote:
> > Hi! Thank you for review! My answers and new patch below.
> > 
> > On Wed, Aug 07, 2019 at 09:25:59PM +0300, n.pettik wrote:
> > > 
> > > > 
> > 
> > New patch:
> > 
> > From d0ffad654fe4ddf366bd4cfd4e1c9340a8044244 Mon Sep 17 00:00:00 2001
> > From: Mergen Imeev <imeevma@gmail.com>
> > Date: Mon, 22 Jul 2019 12:54:34 +0300
> > Subject: [PATCH] sql: make valueToText() operate on MAP/ARRAY values
> > 
> > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > index 5516d7f..f94f118 100644
> > --- a/src/box/sql/vdbemem.c
> > +++ b/src/box/sql/vdbemem.c
> > @@ -281,15 +281,30 @@ int
> >  sqlVdbeMemStringify(Mem * pMem)
> >  {
> >  	int fg = pMem->flags;
> > -	const int nByte = 32;
> > +	int nByte = 32;
> >  
> > -	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
> > +	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
> > +	    ((pMem->flags & MEM_Subtype) == 0 ||
> > +	     pMem->subtype != SQL_SUBTYPE_MSGPACK))
> >  		return 0;
> 
> Why do you need OR condition? There's no any other subtype except for
> SQL_SUBTYPE_MSGPACK. If the latter is set, MEM_Subtype must be set.
> 
I am not sure about that since subtype is checked only when
MEM_Subtype is set. It is unknown what value subtype field
has when MEM_Subtype is not set. I replaced this check with
new helper function.

> >  
> > +		memcpy(value, value_str, nByte);
> > +	}
> > +
> >  	if (sqlVdbeMemClearAndResize(pMem, nByte)) {
> >  		return -1;
> >  	}
> > @@ -302,6 +317,11 @@ sqlVdbeMemStringify(Mem * pMem)
> >  	} else if ((fg & MEM_Bool) != 0) {
> >  		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
> >  		pMem->flags &= ~MEM_Bool;
> > +	} else if ((fg & MEM_Subtype) != 0 &&
> > +		   pMem->subtype == SQL_SUBTYPE_MSGPACK) {
> > +		sql_snprintf(nByte, pMem->z, "%s", value);
> > +		pMem->flags &= ~MEM_Subtype;
> > +		pMem->subtype = SQL_SUBTYPE_NO;
> >  	} else {
> >  		assert(fg & MEM_Real);
> >  		sql_snprintf(nByte, pMem->z, "%!.15g", pMem->u.r);
> > @@ -1148,7 +1168,9 @@ valueToText(sql_value * pVal)
> >  {
> >  	assert(pVal != 0);
> >  	assert((pVal->flags & (MEM_Null)) == 0);
> > -	if (pVal->flags & (MEM_Blob | MEM_Str)) {
> > +	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
> > +	    ((pVal->flags & MEM_Subtype) == 0 ||
> > +	     pVal->subtype != SQL_SUBTYPE_MSGPACK)) {
> 
> You quite intensively abuse this check, so I'd better introduce
> helper function for that purpose.
> 
Done.

> >  		if (ExpandBlob(pVal))
> >  			return 0;
> >  		pVal->flags |= MEM_Str;
> > diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> > index f6a2dfd..c9f0952 100644
> > --- a/test/sql/types.test.lua
> > +++ b/test/sql/types.test.lua
> > @@ -512,3 +512,26 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
> >  box.execute('UPDATE t SET b = false WHERE i = 1;')
> >  s:select()
> >  s:drop()
> > +
> > +--
> > +-- Make sure that the array/map conversion to scalar error is
> > +-- displayed correctly.
> > +--
> > +box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
> > +format = {}
> > +format[1] = {type = 'integer', name = 'I'}
> > +format[2] = {type = 'array', name = 'A'}
> > +s = box.schema.space.create('T2', {format=format})
> > +i = s:create_index('ii')
> > +s:insert({1, {1,2,3}})
> > +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> > +s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
> > +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> > +s:drop()
> > +format[2].type = 'map'
> > +s = box.schema.space.create('T2', {format=format})
> > +i = s:create_index('ii')
> > +s:insert({1, {b = 1}})
> > +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> > +s:drop()
> > +box.execute('DROP TABLE t1;')
> 
> I'd also add synthetic test verifing that huge enough tuples (string
> representation of which doesn't fit into static buffer) are processed
> without accidents.
> 
> 
Added. Still, the error doesn't look right actually, but
this problem I am going to solve for this errcode outside
of this issue.


New patch:

From 79932ca82d6fc3999ffedf707ac01bf2a17e9859 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 22 Jul 2019 12:54:34 +0300
Subject: [PATCH] sql: make valueToText() operate on MAP/ARRAY values

Since ARRAY and MAP cannot be converted to SCALAR type, this
operation should throw an error. But when the error is raised in
SQL, it is displayed in unreadable form. The reason for this is
that the given array or map is not correctly converted to a
string. This patch fixes the problem by converting ARRAY or MAP to
their string representation.
For example:

box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
format = {}
format[1] = {type = 'integer', name = 'I'}
format[2] = {type = 'array', name = 'A'}
s = box.schema.space.create('T2', {format=format})
i = s:create_index('ii')
s:insert({1, {1,2,3}})
box.execute('INSERT INTO t1 SELECT * FROM t2;')

Should return:
- error: 'Type mismatch: can not convert [1, 2, 3] to scalar'

Follow-up #4189

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index ac0dfa3..7c15155 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -263,6 +263,13 @@ sqlVdbeMemNulTerminate(Mem * pMem)
 	}
 }
 
+static inline bool
+sql_mem_has_msgpack_subtype(struct Mem *mem)
+{
+	return (mem->flags & MEM_Subtype) != 0 &&
+	       mem->subtype == SQL_SUBTYPE_MSGPACK;
+}
+
 /*
  * Add MEM_Str to the set of representations for the given Mem.  Numbers
  * are converted using sql_snprintf().  Converting a BLOB to a string
@@ -281,15 +288,29 @@ int
 sqlVdbeMemStringify(Mem * pMem)
 {
 	int fg = pMem->flags;
-	const int nByte = 32;
+	int nByte = 32;
 
-	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
+	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
+	    !sql_mem_has_msgpack_subtype(pMem))
 		return 0;
 
 	assert(!(fg & MEM_Zero));
-	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool)) != 0);
+	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool |
+		      MEM_Subtype)) != 0);
 	assert(EIGHT_BYTE_ALIGNMENT(pMem));
 
+	/*
+	 * In case we have ARRAY/MAP we should save decoded value
+	 * before clearing pMem->z.
+	 */
+	char *value = NULL;
+	if (sql_mem_has_msgpack_subtype(pMem)) {
+		const char *value_str = mp_str(pMem->z);
+		nByte = strlen(value_str) + 1;
+		value = region_alloc(&fiber()->gc, nByte);
+		memcpy(value, value_str, nByte);
+	}
+
 	if (sqlVdbeMemClearAndResize(pMem, nByte)) {
 		return -1;
 	}
@@ -302,6 +323,10 @@ sqlVdbeMemStringify(Mem * pMem)
 	} else if ((fg & MEM_Bool) != 0) {
 		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
 		pMem->flags &= ~MEM_Bool;
+	} else if (sql_mem_has_msgpack_subtype(pMem)) {
+		sql_snprintf(nByte, pMem->z, "%s", value);
+		pMem->flags &= ~MEM_Subtype;
+		pMem->subtype = SQL_SUBTYPE_NO;
 	} else {
 		assert(fg & MEM_Real);
 		sql_snprintf(nByte, pMem->z, "%!.15g", pMem->u.r);
@@ -1140,7 +1165,8 @@ valueToText(sql_value * pVal)
 {
 	assert(pVal != 0);
 	assert((pVal->flags & (MEM_Null)) == 0);
-	if (pVal->flags & (MEM_Blob | MEM_Str)) {
+	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
+	    !sql_mem_has_msgpack_subtype(pVal)) {
 		if (ExpandBlob(pVal))
 			return 0;
 		pVal->flags |= MEM_Str;
diff --git a/test/sql/types.result b/test/sql/types.result
index 1241ae4..5f0d181 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -2266,3 +2266,101 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+---
+- row_count: 1
+...
+format = {}
+---
+...
+format[1] = {type = 'integer', name = 'I'}
+---
+...
+format[2] = {type = 'array', name = 'A'}
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert [1, 2, 3] to scalar'
+...
+s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
+---
+- [1, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,
+    22, 23, 24, 25, 26, 27, 28, 29, 30]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
+  15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30] to scalar'
+...
+long_array = {}
+---
+...
+for i = 1,120 do long_array[i] = i end
+---
+...
+s:replace({1, long_array})
+---
+- [1, [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21,
+    22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41,
+    42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61,
+    62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81,
+    82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101,
+    102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117,
+    118, 119, 120]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
+  15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34,
+  35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54,
+  55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74,
+  75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94,
+  95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111,
+  112, 113, 114, 115, 116, 117, 11'
+...
+s:drop()
+---
+...
+format[2].type = 'map'
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {b = 1}})
+---
+- [1, {'b': 1}]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- null
+- 'Type mismatch: can not convert {"b": 1} to scalar'
+...
+s:drop()
+---
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index f6a2dfd..b0e235c 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -512,3 +512,30 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
 box.execute('UPDATE t SET b = false WHERE i = 1;')
 s:select()
 s:drop()
+
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+format = {}
+format[1] = {type = 'integer', name = 'I'}
+format[2] = {type = 'array', name = 'A'}
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {1,2,3}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+long_array = {}
+for i = 1,120 do long_array[i] = i end
+s:replace({1, long_array})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+format[2].type = 'map'
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {b = 1}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+box.execute('DROP TABLE t1;')

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[korablev]

On Mon, Sep 02, 2019 at 04:53:13PM +0300, Mergen Imeev wrote:
> > > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > > index 5516d7f..f94f118 100644
> > > --- a/src/box/sql/vdbemem.c
> > > +++ b/src/box/sql/vdbemem.c
> > > @@ -281,15 +281,30 @@ int
> > >  sqlVdbeMemStringify(Mem * pMem)
> > >  {
> > >  	int fg = pMem->flags;
> > > -	const int nByte = 32;
> > > +	int nByte = 32;
> > >  
> > > -	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
> > > +	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
> > > +	    ((pMem->flags & MEM_Subtype) == 0 ||
> > > +	     pMem->subtype != SQL_SUBTYPE_MSGPACK))
> > >  		return 0;
> > 
> > Why do you need OR condition? There's no any other subtype except for
> > SQL_SUBTYPE_MSGPACK. If the latter is set, MEM_Subtype must be set.
> > 
> I am not sure about that since subtype is checked only when
> MEM_Subtype is set. It is unknown what value subtype field
> has when MEM_Subtype is not set. I replaced this check with
> new helper function.

Yep, so according to this logic you can short-cut your to:

... && (pMem->flags & MEM_Subtype) == 0

Since if MEM_Subtype is set, then the only allowed subtype is
SQL_SUBTYPE_MSGPACK.

> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index ac0dfa3..7c15155 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -263,6 +263,13 @@ sqlVdbeMemNulTerminate(Mem * pMem)
>  	}
>  }
>  
> +static inline bool
> +sql_mem_has_msgpack_subtype(struct Mem *mem)

Nit: there's no need in 'sql_' prefix in case function is static.
Please, remove and rename to mem_has_msgpack_subtype()

> +{
> +	return (mem->flags & MEM_Subtype) != 0 &&
> +	       mem->subtype == SQL_SUBTYPE_MSGPACK;
> +}
> +
>  /*
>   * Add MEM_Str to the set of representations for the given Mem.  Numbers
>   * are converted using sql_snprintf().  Converting a BLOB to a string
> @@ -281,15 +288,29 @@ int
>  sqlVdbeMemStringify(Mem * pMem)
>  {
>  	int fg = pMem->flags;
> -	const int nByte = 32;
> +	int nByte = 32;
>  
> -	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
> +	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
> +	    !sql_mem_has_msgpack_subtype(pMem))
>  		return 0;
>  
>  	assert(!(fg & MEM_Zero));
> -	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool)) != 0);
> +	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool |
> +		      MEM_Subtype)) != 0);

Nit: this assertion verifies type correctness, so intead of checking
that MEM_Subtype is set I'd rather check that MEM_Blob is set.

> @@ -302,6 +323,10 @@ sqlVdbeMemStringify(Mem * pMem)
>  	} else if ((fg & MEM_Bool) != 0) {
>  		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
>  		pMem->flags &= ~MEM_Bool;
> +	} else if (sql_mem_has_msgpack_subtype(pMem)) {
> +		sql_snprintf(nByte, pMem->z, "%s", value);
> +		pMem->flags &= ~MEM_Subtype;
> +		pMem->subtype = SQL_SUBTYPE_NO;
>  	} else {
>  		assert(fg & MEM_Real);
>  		sql_snprintf(nByte, pMem->z, "%!.15g", pMem->u.r);
> @@ -1140,7 +1165,8 @@ valueToText(sql_value * pVal)
>  {
>  	assert(pVal != 0);
>  	assert((pVal->flags & (MEM_Null)) == 0);
> -	if (pVal->flags & (MEM_Blob | MEM_Str)) {
> +	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
> +	    !sql_mem_has_msgpack_subtype(pVal)) {
>  		if (ExpandBlob(pVal))
>  			return 0;
>  		pVal->flags |= MEM_Str;
> diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> index f6a2dfd..b0e235c 100644
> --- a/test/sql/types.test.lua
> +++ b/test/sql/types.test.lua
> @@ -512,3 +512,30 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
>  box.execute('UPDATE t SET b = false WHERE i = 1;')
>  s:select()
>  s:drop()
> +
> +--
> +-- Make sure that the array/map conversion to scalar error is
> +-- displayed correctly.
> +--
> +box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
> +format = {}
> +format[1] = {type = 'integer', name = 'I'}
> +format[2] = {type = 'array', name = 'A'}
> +s = box.schema.space.create('T2', {format=format})
> +i = s:create_index('ii')
> +s:insert({1, {1,2,3}})
> +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> +s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
> +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')

Nit: please, add comment explaining what test below verifies.

> +long_array = {}
> +for i = 1,120 do long_array[i] = i end
> +s:replace({1, long_array})
> +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> +s:drop()
> +format[2].type = 'map'
> +s = box.schema.space.create('T2', {format=format})
> +i = s:create_index('ii')
> +s:insert({1, {b = 1}})
> +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> +s:drop()
> +box.execute('DROP TABLE t1;')

The rest is OK. Please, deal with nits and then patch can be pushed.

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Mergen Imeev]

Hi! Thank you for review. I fixed all nits. Diff below.

On Tue, Sep 10, 2019 at 04:54:46PM +0300, korablev@tarantool.org wrote:
> On Mon, Sep 02, 2019 at 04:53:13PM +0300, Mergen Imeev wrote:
> > > > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > > > index 5516d7f..f94f118 100644
> > > > --- a/src/box/sql/vdbemem.c
> > > > +++ b/src/box/sql/vdbemem.c
> > > > @@ -281,15 +281,30 @@ int
> > > >  sqlVdbeMemStringify(Mem * pMem)
> > > >  {
> > > >  	int fg = pMem->flags;
> > > > -	const int nByte = 32;
> > > > +	int nByte = 32;
> > > >  
> > > > -	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
> > > > +	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
> > > > +	    ((pMem->flags & MEM_Subtype) == 0 ||
> > > > +	     pMem->subtype != SQL_SUBTYPE_MSGPACK))
> > > >  		return 0;
> > > 
> > > Why do you need OR condition? There's no any other subtype except for
> > > SQL_SUBTYPE_MSGPACK. If the latter is set, MEM_Subtype must be set.
> > > 
> > I am not sure about that since subtype is checked only when
> > MEM_Subtype is set. It is unknown what value subtype field
> > has when MEM_Subtype is not set. I replaced this check with
> > new helper function.
> 
> Yep, so according to this logic you can short-cut your to:
> 
> ... && (pMem->flags & MEM_Subtype) == 0
> 
> Since if MEM_Subtype is set, then the only allowed subtype is
> SQL_SUBTYPE_MSGPACK.
> 
> > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > index ac0dfa3..7c15155 100644
> > --- a/src/box/sql/vdbemem.c
> > +++ b/src/box/sql/vdbemem.c
> > @@ -263,6 +263,13 @@ sqlVdbeMemNulTerminate(Mem * pMem)
> >  	}
> >  }
> >  
> > +static inline bool
> > +sql_mem_has_msgpack_subtype(struct Mem *mem)
> 
> Nit: there's no need in 'sql_' prefix in case function is static.
> Please, remove and rename to mem_has_msgpack_subtype()
> 
> > +{
> > +	return (mem->flags & MEM_Subtype) != 0 &&
> > +	       mem->subtype == SQL_SUBTYPE_MSGPACK;
> > +}
> > +
> >  /*
> >   * Add MEM_Str to the set of representations for the given Mem.  Numbers
> >   * are converted using sql_snprintf().  Converting a BLOB to a string
> > @@ -281,15 +288,29 @@ int
> >  sqlVdbeMemStringify(Mem * pMem)
> >  {
> >  	int fg = pMem->flags;
> > -	const int nByte = 32;
> > +	int nByte = 32;
> >  
> > -	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
> > +	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
> > +	    !sql_mem_has_msgpack_subtype(pMem))
> >  		return 0;
> >  
> >  	assert(!(fg & MEM_Zero));
> > -	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool)) != 0);
> > +	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool |
> > +		      MEM_Subtype)) != 0);
> 
> Nit: this assertion verifies type correctness, so intead of checking
> that MEM_Subtype is set I'd rather check that MEM_Blob is set.
> 
> > @@ -302,6 +323,10 @@ sqlVdbeMemStringify(Mem * pMem)
> >  	} else if ((fg & MEM_Bool) != 0) {
> >  		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
> >  		pMem->flags &= ~MEM_Bool;
> > +	} else if (sql_mem_has_msgpack_subtype(pMem)) {
> > +		sql_snprintf(nByte, pMem->z, "%s", value);
> > +		pMem->flags &= ~MEM_Subtype;
> > +		pMem->subtype = SQL_SUBTYPE_NO;
> >  	} else {
> >  		assert(fg & MEM_Real);
> >  		sql_snprintf(nByte, pMem->z, "%!.15g", pMem->u.r);
> > @@ -1140,7 +1165,8 @@ valueToText(sql_value * pVal)
> >  {
> >  	assert(pVal != 0);
> >  	assert((pVal->flags & (MEM_Null)) == 0);
> > -	if (pVal->flags & (MEM_Blob | MEM_Str)) {
> > +	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
> > +	    !sql_mem_has_msgpack_subtype(pVal)) {
> >  		if (ExpandBlob(pVal))
> >  			return 0;
> >  		pVal->flags |= MEM_Str;
> > diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> > index f6a2dfd..b0e235c 100644
> > --- a/test/sql/types.test.lua
> > +++ b/test/sql/types.test.lua
> > @@ -512,3 +512,30 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
> >  box.execute('UPDATE t SET b = false WHERE i = 1;')
> >  s:select()
> >  s:drop()
> > +
> > +--
> > +-- Make sure that the array/map conversion to scalar error is
> > +-- displayed correctly.
> > +--
> > +box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
> > +format = {}
> > +format[1] = {type = 'integer', name = 'I'}
> > +format[2] = {type = 'array', name = 'A'}
> > +s = box.schema.space.create('T2', {format=format})
> > +i = s:create_index('ii')
> > +s:insert({1, {1,2,3}})
> > +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> > +s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
> > +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> 
> Nit: please, add comment explaining what test below verifies.
> 
> > +long_array = {}
> > +for i = 1,120 do long_array[i] = i end
> > +s:replace({1, long_array})
> > +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> > +s:drop()
> > +format[2].type = 'map'
> > +s = box.schema.space.create('T2', {format=format})
> > +i = s:create_index('ii')
> > +s:insert({1, {b = 1}})
> > +box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
> > +s:drop()
> > +box.execute('DROP TABLE t1;')
> 
> The rest is OK. Please, deal with nits and then patch can be pushed.
> 

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 7c15155..acffa97 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -264,7 +264,7 @@ sqlVdbeMemNulTerminate(Mem * pMem)
 }
 
 static inline bool
-sql_mem_has_msgpack_subtype(struct Mem *mem)
+mem_has_msgpack_subtype(struct Mem *mem)
 {
 	return (mem->flags & MEM_Subtype) != 0 &&
 	       mem->subtype == SQL_SUBTYPE_MSGPACK;
@@ -291,12 +291,12 @@ sqlVdbeMemStringify(Mem * pMem)
 	int nByte = 32;
 
 	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0 &&
-	    !sql_mem_has_msgpack_subtype(pMem))
+	    !mem_has_msgpack_subtype(pMem))
 		return 0;
 
 	assert(!(fg & MEM_Zero));
 	assert((fg & (MEM_Int | MEM_UInt | MEM_Real | MEM_Bool |
-		      MEM_Subtype)) != 0);
+		      MEM_Blob)) != 0);
 	assert(EIGHT_BYTE_ALIGNMENT(pMem));
 
 	/*
@@ -304,7 +304,7 @@ sqlVdbeMemStringify(Mem * pMem)
 	 * before clearing pMem->z.
 	 */
 	char *value = NULL;
-	if (sql_mem_has_msgpack_subtype(pMem)) {
+	if (mem_has_msgpack_subtype(pMem)) {
 		const char *value_str = mp_str(pMem->z);
 		nByte = strlen(value_str) + 1;
 		value = region_alloc(&fiber()->gc, nByte);
@@ -323,7 +323,7 @@ sqlVdbeMemStringify(Mem * pMem)
 	} else if ((fg & MEM_Bool) != 0) {
 		sql_snprintf(nByte, pMem->z, "%s", pMem->u.b ? "true" : "false");
 		pMem->flags &= ~MEM_Bool;
-	} else if (sql_mem_has_msgpack_subtype(pMem)) {
+	} else if (mem_has_msgpack_subtype(pMem)) {
 		sql_snprintf(nByte, pMem->z, "%s", value);
 		pMem->flags &= ~MEM_Subtype;
 		pMem->subtype = SQL_SUBTYPE_NO;
@@ -1166,7 +1166,7 @@ valueToText(sql_value * pVal)
 	assert(pVal != 0);
 	assert((pVal->flags & (MEM_Null)) == 0);
 	if ((pVal->flags & (MEM_Blob | MEM_Str)) &&
-	    !sql_mem_has_msgpack_subtype(pVal)) {
+	    !mem_has_msgpack_subtype(pVal)) {
 		if (ExpandBlob(pVal))
 			return 0;
 		pVal->flags |= MEM_Str;
@@ -1835,8 +1835,7 @@ encode_uint:
 		 * Emit BIN header iff the BLOB doesn't store
 		 * MsgPack content.
 		 */
-		if ((var->flags & MEM_Subtype) == 0 ||
-		     var->subtype != SQL_SUBTYPE_MSGPACK) {
+		if (!mem_has_msgpack_subtype(var)) {
 			uint32_t binl = var->n +
 					((var->flags & MEM_Zero) ?
 					var->u.nZero : 0);
diff --git a/test/sql/types.result b/test/sql/types.result
index 5f0d181..a4fd2f7 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -2309,6 +2309,10 @@ box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
 - 'Type mismatch: can not convert [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14,
   15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30] to scalar'
 ...
+--
+-- Make sure that the error will be displayed correctly even if
+-- the value is too long.
+--
 long_array = {}
 ---
 ...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index b0e235c..5c99cfc 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -527,6 +527,10 @@ s:insert({1, {1,2,3}})
 box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
 s:replace({1, {1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30}})
 box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+--
+-- Make sure that the error will be displayed correctly even if
+-- the value is too long.
+--
 long_array = {}
 for i = 1,120 do long_array[i] = i end
 s:replace({1, long_array})

[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Nikita Pettik]

On Wed, Sep 11, 2019 at 11:13:13AM +0300, Mergen Imeev wrote:
> Hi! Thank you for review. I fixed all nits. Diff below.

Thanks, LGTM now.