Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: imeevma@gmail.com
Subject: [tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR
Date: Thu, 29 Aug 2019 15:19:30 +0300	[thread overview]
Message-ID: <20190829121930.GA51601@tarantool.org> (raw)
In-Reply-To: <20190828133002.GA17433@tarantool.org>

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.

  reply	other threads:[~2019-08-29 12:19 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-24  8:11 [tarantool-patches] [PATCH v2 0/2] sql: add ARRAY, MAP and ANY types to mem_apply_type() imeevma
2019-07-24  8:12 ` [tarantool-patches] [PATCH v2 1/2] " imeevma
2019-07-24  8:12 ` [tarantool-patches] [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR imeevma
2019-07-24 12:24   ` [tarantool-patches] " n.pettik
2019-07-24 22:37     ` Konstantin Osipov
2019-07-24 23:30       ` n.pettik
2019-07-27 10:16     ` Mergen Imeev
2019-08-07 18:25       ` n.pettik
2019-08-28 13:30         ` Mergen Imeev
2019-08-29 12:19           ` Nikita Pettik [this message]
2019-09-02 13:53             ` Mergen Imeev
2019-09-10 13:54               ` korablev
2019-09-11  8:13                 ` Mergen Imeev
2019-09-11  9:25                   ` Nikita Pettik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190829121930.GA51601@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=imeevma@gmail.com \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox