[tarantool-patches] [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[tarantool-patches] [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[imeevma]

Before this patch, UPDATE throwed an error when executed on a
tuple with types unknown to SQL. This patch fixes the problem in
case the fields with unknown types are not updated.

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

 src/box/sql/vdbe.c      | 25 +++++++++++++++++++++++++
 test/sql/types.result   | 43 +++++++++++++++++++++++++++++++++++++++++++
 test/sql/types.test.lua | 17 +++++++++++++++++
 3 files changed, 85 insertions(+)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..8ddc29d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -294,6 +294,20 @@ mem_apply_numeric_type(struct Mem *record)
  * SCALAR:
  *    Mem is unchanged, but flag is set to BLOB.
  *
+ * BOOLEAN:
+ *    If memory holds BOOLEAN no actions take place.
+ *
+ * ANY:
+ *    Mem is unchanged, no actions take place.
+ *
+ * MAP:
+ *    If memory holds value with SQL_SUBTYPE_MSGPACK subtype and
+ *    data has MP_MAP type, no actions take place.
+ *
+ * ARRAY:
+ *    If memory holds value with SQL_SUBTYPE_MSGPACK subtype and
+ *    data has MP_ARRAY type, no actions take place.
+ *
  * @param record The value to apply type to.
  * @param type The type to be applied.
  */
@@ -337,8 +351,19 @@ mem_apply_type(struct Mem *record, enum field_type type)
 		}
 		record->flags &= ~(MEM_Real | MEM_Int);
 		return 0;
+	case FIELD_TYPE_ANY:
 	case FIELD_TYPE_SCALAR:
 		return 0;
+	case FIELD_TYPE_MAP:
+		if (record->subtype == SQL_SUBTYPE_MSGPACK &&
+		    mp_typeof(*record->z) == MP_MAP)
+			return 0;
+		return -1;
+	case FIELD_TYPE_ARRAY:
+		if (record->subtype == SQL_SUBTYPE_MSGPACK &&
+		    mp_typeof(*record->z) == MP_ARRAY)
+			return 0;
+		return -1;
 	default:
 		return -1;
 	}
diff --git a/test/sql/types.result b/test/sql/types.result
index cdfb1e7..5cc7f16 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -966,3 +966,46 @@ box.execute('SELECT ?', {true})
   rows:
   - [true]
 ...
+--
+-- gh-4189: Update throws an error when executed on a tuple with
+-- types unknown to SQL.
+--
+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 ae1a0ab..d33314a 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -234,3 +234,20 @@ box.execute('SELECT \'9223372036854\' + 1;')
 
 -- Fix BOOLEAN bindings.
 box.execute('SELECT ?', {true})
+
+--
+-- gh-4189: Update throws an error when executed on a tuple with
+-- types unknown to SQL.
+--
+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] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[n.pettik]

> On 28 Jun 2019, at 18:34, imeevma@tarantool.org wrote:
> 
> Before this patch, UPDATE throwed an error when executed on a
> tuple with types unknown to SQL. This patch fixes the problem in
> case the fields with unknown types are not updated.

I’ve fixed commit message a bit:

sql: add ARRAY, MAP and ANY types to mem_apply_type()

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
> ---
> https://github.com/tarantool/tarantool/issues/4189
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4189-field-type-conversion-error

I suggest follow refactoring:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 8ddc29d14..f8cf1afc7 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -292,7 +292,9 @@ 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.
@@ -300,13 +302,9 @@ mem_apply_numeric_type(struct Mem *record)
  * ANY:
  *    Mem is unchanged, no actions take place.
  *
- * MAP:
- *    If memory holds value with SQL_SUBTYPE_MSGPACK subtype and
- *    data has MP_MAP type, no actions take place.
- *
- * ARRAY:
- *    If memory holds value with SQL_SUBTYPE_MSGPACK subtype and
- *    data has MP_ARRAY type, 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.
@@ -351,8 +349,13 @@ mem_apply_type(struct Mem *record, enum field_type type)
                }
                record->flags &= ~(MEM_Real | MEM_Int);
                return 0;
-       case FIELD_TYPE_ANY:
        case FIELD_TYPE_SCALAR:
+               /* Can't cast MAP and ARRAY to scalar types. */
+               if (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->subtype == SQL_SUBTYPE_MSGPACK &&
@@ -364,6 +367,8 @@ mem_apply_type(struct Mem *record, enum field_type type)
                    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 5cc7f1659..32588d38c 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -967,8 +967,8 @@ box.execute('SELECT ?', {true})
   - [true]
 ...
 --
--- gh-4189: Update throws an error when executed on a tuple with
--- types unknown to SQL.
+-- gh-4189: make sure that update doesn't throw an error if format
+-- of table features map/array field types.
 --
 format = {}
 ---
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index d33314a0b..69af421d2 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -236,8 +236,8 @@ box.execute('SELECT \'9223372036854\' + 1;')
 box.execute('SELECT ?', {true})
 
 --
--- gh-4189: Update throws an error when executed on a tuple with
--- types unknown to SQL.
+-- 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'}

[tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[Mergen Imeev]

Hi! Thank you for review and fixes. I squashed you fixes
and made a few changes. New patch and diff for these
changes below.

On Tue, Jul 02, 2019 at 02:29:14AM +0300, n.pettik wrote:
> 
> 
> > On 28 Jun 2019, at 18:34, imeevma@tarantool.org wrote:
> > 
> > Before this patch, UPDATE throwed an error when executed on a
> > tuple with types unknown to SQL. This patch fixes the problem in
> > case the fields with unknown types are not updated.
> 
> I’ve fixed commit message a bit:
> 
> sql: add ARRAY, MAP and ANY types to mem_apply_type()
> 
> 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.  
> 
Thanks, fixed.

> > Closes #4189
> > ---
> > https://github.com/tarantool/tarantool/issues/4189
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-4189-field-type-conversion-error
> 
> I suggest follow refactoring:
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 8ddc29d14..f8cf1afc7 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -292,7 +292,9 @@ 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.
> @@ -300,13 +302,9 @@ mem_apply_numeric_type(struct Mem *record)
>   * ANY:
>   *    Mem is unchanged, no actions take place.
>   *
> - * MAP:
> - *    If memory holds value with SQL_SUBTYPE_MSGPACK subtype and
> - *    data has MP_MAP type, no actions take place.
> - *
> - * ARRAY:
> - *    If memory holds value with SQL_SUBTYPE_MSGPACK subtype and
> - *    data has MP_ARRAY type, 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.
> @@ -351,8 +349,13 @@ mem_apply_type(struct Mem *record, enum field_type type)
>                 }
>                 record->flags &= ~(MEM_Real | MEM_Int);
>                 return 0;
> -       case FIELD_TYPE_ANY:
>         case FIELD_TYPE_SCALAR:
> +               /* Can't cast MAP and ARRAY to scalar types. */
> +               if (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->subtype == SQL_SUBTYPE_MSGPACK &&
> @@ -364,6 +367,8 @@ mem_apply_type(struct Mem *record, enum field_type type)
>                     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 5cc7f1659..32588d38c 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -967,8 +967,8 @@ box.execute('SELECT ?', {true})
>    - [true]
>  ...
>  --
> --- gh-4189: Update throws an error when executed on a tuple with
> --- types unknown to SQL.
> +-- gh-4189: make sure that update doesn't throw an error if format
> +-- of table features map/array field types.
>  --
>  format = {}
>  ---
> diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> index d33314a0b..69af421d2 100644
> --- a/test/sql/types.test.lua
> +++ b/test/sql/types.test.lua
> @@ -236,8 +236,8 @@ box.execute('SELECT \'9223372036854\' + 1;')
>  box.execute('SELECT ?', {true})
>  
>  --
> --- gh-4189: Update throws an error when executed on a tuple with
> --- types unknown to SQL.
> +-- 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'}
> 
Thanks, squashed.

Diff:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index f8cf1af..79232de 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -351,19 +351,22 @@ mem_apply_type(struct Mem *record, enum field_type type)
 		return 0;
 	case FIELD_TYPE_SCALAR:
 		/* Can't cast MAP and ARRAY to scalar types. */
-		if (record->subtype == SQL_SUBTYPE_MSGPACK) {
+		if ((record->flags & MEM_Blob) == MEM_Blob &&
+		    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->subtype == SQL_SUBTYPE_MSGPACK &&
+		if ((record->flags & MEM_Blob) == MEM_Blob &&
+		    record->subtype == SQL_SUBTYPE_MSGPACK &&
 		    mp_typeof(*record->z) == MP_MAP)
 			return 0;
 		return -1;
 	case FIELD_TYPE_ARRAY:
-		if (record->subtype == SQL_SUBTYPE_MSGPACK &&
+		if ((record->flags & MEM_Blob) == MEM_Blob &&
+		    record->subtype == SQL_SUBTYPE_MSGPACK &&
 		    mp_typeof(*record->z) == MP_ARRAY)
 			return 0;
 		return -1;


New patch:

From 5c04d08a5d6c88ea25b60b931e83bfedde5ae27e Mon Sep 17 00:00:00 2001
Date: Fri, 28 Jun 2019 18:16:16 +0300
Subject: [PATCH] sql: add ARRAY, MAP and ANY types to mem_apply_type()

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

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index c8887f9..79232de 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_Blob) == MEM_Blob &&
+		    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_Blob) == MEM_Blob &&
+		    record->subtype == SQL_SUBTYPE_MSGPACK &&
+		    mp_typeof(*record->z) == MP_MAP)
+			return 0;
+		return -1;
+	case FIELD_TYPE_ARRAY:
+		if ((record->flags & MEM_Blob) == MEM_Blob &&
+		    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 cdfb1e7..32588d3 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -966,3 +966,46 @@ box.execute('SELECT ?', {true})
   rows:
   - [true]
 ...
+--
+-- 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 ae1a0ab..69af421 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -234,3 +234,20 @@ box.execute('SELECT \'9223372036854\' + 1;')
 
 -- Fix BOOLEAN bindings.
 box.execute('SELECT ?', {true})
+
+--
+-- 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()

[tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]


> Diff:
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index f8cf1af..79232de 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -351,19 +351,22 @@ mem_apply_type(struct Mem *record, enum field_type type)
> 		return 0;
> 	case FIELD_TYPE_SCALAR:
> 		/* Can't cast MAP and ARRAY to scalar types. */
> -		if (record->subtype == SQL_SUBTYPE_MSGPACK) {
> +		if ((record->flags & MEM_Blob) == MEM_Blob &&

Why do you need this additional check on MEM_Blob?
Is it possible that memory holds raw msgpack and its
type not blob? If so, please provide an example.

> +		    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->subtype == SQL_SUBTYPE_MSGPACK &&
> +		if ((record->flags & MEM_Blob) == MEM_Blob &&
> +		    record->subtype == SQL_SUBTYPE_MSGPACK &&
> 		    mp_typeof(*record->z) == MP_MAP)
> 			return 0;
> 		return -1;
> 	case FIELD_TYPE_ARRAY:
> -		if (record->subtype == SQL_SUBTYPE_MSGPACK &&
> +		if ((record->flags & MEM_Blob) == MEM_Blob &&
> +		    record->subtype == SQL_SUBTYPE_MSGPACK &&
> 		    mp_typeof(*record->z) == MP_ARRAY)
> 			return 0;
> 		return -1;


[-- Attachment #2: Type: text/html, Size: 46669 bytes --]

[tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

Hi! Thank you for review. My answer below.

On 7/8/19 4:32 PM, n.pettik wrote:
>
>> Diff:
>>
>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>> index f8cf1af..79232de 100644
>> --- a/src/box/sql/vdbe.c
>> +++ b/src/box/sql/vdbe.c
>> @@ -351,19 +351,22 @@ mem_apply_type(struct Mem *record, enum 
>> field_type type)
>> return 0;
>> case FIELD_TYPE_SCALAR:
>> /* Can't cast MAP and ARRAY to scalar types. */
>> -if (record->subtype == SQL_SUBTYPE_MSGPACK) {
>> +if ((record->flags & MEM_Blob) == MEM_Blob &&
>
> Why do you need this additional check on MEM_Blob?
> Is it possible that memory holds raw msgpack and its
> type not blob? If so, please provide an example.
>
I'm not sure what you describe is possible. But it is impossible
to say what type MEM has when you look at the SUBTYPE. At the same
time, the subtype has any meaning only for BLOBs. Any other type
has any SUBTYPE, and it is not checked anywhere. So, to avoid an
error, when something, for example INT, throws an error when
casting to SCALAR, I added these checks.


[-- Attachment #2: Type: text/html, Size: 16646 bytes --]

[tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[n.pettik]

> On 9 Jul 2019, at 12:14, Imeev Mergen <imeevma@tarantool.org> wrote:
> 
> Hi! Thank you for review. My answer below.
> 
> On 7/8/19 4:32 PM, n.pettik wrote:
>> 
>>> Diff:
>>> 
>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>> index f8cf1af..79232de 100644
>>> --- a/src/box/sql/vdbe.c
>>> +++ b/src/box/sql/vdbe.c
>>> @@ -351,19 +351,22 @@ mem_apply_type(struct Mem *record, enum field_type type)
>>> 		return 0;
>>> 	case FIELD_TYPE_SCALAR:
>>> 		/* Can't cast MAP and ARRAY to scalar types. */
>>> -		if (record->subtype == SQL_SUBTYPE_MSGPACK) {
>>> +		if ((record->flags & MEM_Blob) == MEM_Blob &&
>> 
>> Why do you need this additional check on MEM_Blob?
>> Is it possible that memory holds raw msgpack and its
>> type not blob? If so, please provide an example.
>> 
> I'm not sure what you describe is possible. But it is impossible
> to say what type MEM has when you look at the SUBTYPE. At the same
> time, the subtype has any meaning only for BLOBs. Any other type
> has any SUBTYPE, and it is not checked anywhere. So, to avoid an
> error, when something, for example INT, throws an error when
> casting to SCALAR, I added these checks.

Ok, but then I guess more appropriate solution would be
checking MEM_Subtype flag, instead of MEM_Blob
Anyway, I would add clear test which reaches this path
and record->flags != MEM_Blob, but ->subtype == msgpack.

[tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[Mergen Imeev]

Hi! Thank you for review. My answers, diff and new patch
below. Also, I fixed error description in case array or
map casted to scalar.

On Mon, Jul 15, 2019 at 04:32:08PM +0300, n.pettik wrote:
> 
> 
> > On 9 Jul 2019, at 12:14, Imeev Mergen <imeevma@tarantool.org> wrote:
> > 
> > Hi! Thank you for review. My answer below.
> > 
> > On 7/8/19 4:32 PM, n.pettik wrote:
> >> 
> >>> Diff:
> >>> 
> >>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> >>> index f8cf1af..79232de 100644
> >>> --- a/src/box/sql/vdbe.c
> >>> +++ b/src/box/sql/vdbe.c
> >>> @@ -351,19 +351,22 @@ mem_apply_type(struct Mem *record, enum field_type type)
> >>> 		return 0;
> >>> 	case FIELD_TYPE_SCALAR:
> >>> 		/* Can't cast MAP and ARRAY to scalar types. */
> >>> -		if (record->subtype == SQL_SUBTYPE_MSGPACK) {
> >>> +		if ((record->flags & MEM_Blob) == MEM_Blob &&
> >> 
> >> Why do you need this additional check on MEM_Blob?
> >> Is it possible that memory holds raw msgpack and its
> >> type not blob? If so, please provide an example.
> >> 
> > I'm not sure what you describe is possible. But it is impossible
> > to say what type MEM has when you look at the SUBTYPE. At the same
> > time, the subtype has any meaning only for BLOBs. Any other type
> > has any SUBTYPE, and it is not checked anywhere. So, to avoid an
> > error, when something, for example INT, throws an error when
> > casting to SCALAR, I added these checks.
> 
> Ok, but then I guess more appropriate solution would be
> checking MEM_Subtype flag, instead of MEM_Blob
Fixed.

> Anyway, I would add clear test which reaches this path
> and record->flags != MEM_Blob, but ->subtype == msgpack.
> 
Sorry, I wasn't able to create reproducer.


Diff:

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 4aba8b4..cf4715d 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -351,7 +351,7 @@ mem_apply_type(struct Mem *record, enum field_type type)
 		return 0;
 	case FIELD_TYPE_SCALAR:
 		/* Can't cast MAP and ARRAY to scalar types. */
-		if ((record->flags & MEM_Blob) == MEM_Blob &&
+		if ((record->flags & MEM_Subtype) != 0 &&
 		    record->subtype == SQL_SUBTYPE_MSGPACK) {
 			assert(mp_typeof(*record->z) == MP_MAP ||
 			       mp_typeof(*record->z) == MP_ARRAY);
@@ -359,13 +359,13 @@ mem_apply_type(struct Mem *record, enum field_type type)
 		}
 		return 0;
 	case FIELD_TYPE_MAP:
-		if ((record->flags & MEM_Blob) == MEM_Blob &&
+		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_Blob) == MEM_Blob &&
+		if ((record->flags & MEM_Subtype) != 0 &&
 		    record->subtype == SQL_SUBTYPE_MSGPACK &&
 		    mp_typeof(*record->z) == MP_ARRAY)
 			return 0;
@@ -2695,8 +2695,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;')


New patch:

From 1d6abdeb18652890c1819349f24d8b450f6ed6c7 Mon Sep 17 00:00:00 2001
Date: Fri, 28 Jun 2019 18:16:16 +0300
Subject: [PATCH] sql: add ARRAY, MAP and ANY types to mem_apply_type()

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

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 9f4ee7a..cf4715d 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;
@@ -2662,8 +2695,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 5abe6e0..61a760f 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1110,6 +1110,108 @@ 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()
+---
+...
+--
+-- 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 410864a..a99cc9f 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -275,4 +275,41 @@ _ = 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()
+
+--
+-- 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;')

[tarantool-patches] Re: [PATCH v1 1/1] sql: Fix UPDATE for types unknown to SQL.

[n.pettik]

[-- Attachment #1: Type: text/plain, Size: 3634 bytes --]


> New patch:
> 
> From 1d6abdeb18652890c1819349f24d8b450f6ed6c7 Mon Sep 17 00:00:00 2001
> Date: Fri, 28 Jun 2019 18:16:16 +0300
> Subject: [PATCH] sql: add ARRAY, MAP and ANY types to mem_apply_type()
> 
> 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
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 9f4ee7a..cf4715d 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;
> @@ -2662,8 +2695,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”;

Please move this fix alongside with test to a separate patch.

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


[-- Attachment #2: Type: text/html, Size: 124921 bytes --]