From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id CFA2422D0F for ; Wed, 11 Sep 2019 04:13:17 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id gYfMSS_VseBp for ; Wed, 11 Sep 2019 04:13:17 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 1B13A22CD6 for ; Wed, 11 Sep 2019 04:13:16 -0400 (EDT) Date: Wed, 11 Sep 2019 11:13:13 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR Message-ID: <20190911081312.GA16521@tarantool.org> References: <67e721f4818f51861383cdf71cc88c63e45ab450.1563955619.git.imeevma@gmail.com> <20190727101643.GA10982@tarantool.org> <20190828133002.GA17433@tarantool.org> <20190829121930.GA51601@tarantool.org> <20190902135312.GA6335@tarantool.org> <20190910135126.GA25212@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190910135126.GA25212@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: korablev@tarantool.org Cc: tarantool-patches@freelists.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})