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

[korablev at tarantool.org]

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.