From: korablev@tarantool.org To: tarantool-patches@freelists.org Cc: imeevma@tarantool.org Subject: [tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR Date: Tue, 10 Sep 2019 16:54:46 +0300 [thread overview] Message-ID: <20190910135126.GA25212@tarantool.org> (raw) In-Reply-To: <20190902135312.GA6335@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.
next prev parent reply other threads:[~2019-09-10 13:54 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 2019-09-02 13:53 ` Mergen Imeev 2019-09-10 13:54 ` korablev [this message] 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=20190910135126.GA25212@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --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