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

[Mergen Imeev <imeevma@tarantool.org>]

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