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

[Mergen Imeev <imeevma@tarantool.org>]

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()