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 3EC9C26E52 for ; Thu, 29 Aug 2019 08:19:33 -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 jPzKN-TF6dBG for ; Thu, 29 Aug 2019 08:19:33 -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 F2B14243EF for ; Thu, 29 Aug 2019 08:19:32 -0400 (EDT) Date: Thu, 29 Aug 2019 15:19:30 +0300 From: Nikita Pettik Subject: [tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR Message-ID: <20190829121930.GA51601@tarantool.org> References: <67e721f4818f51861383cdf71cc88c63e45ab450.1563955619.git.imeevma@gmail.com> <20190727101643.GA10982@tarantool.org> <20190828133002.GA17433@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190828133002.GA17433@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: tarantool-patches@freelists.org Cc: imeevma@gmail.com 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 > 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.