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 9AD88285FC for ; Mon, 2 Sep 2019 09:53: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 8ToKKU6rCZY6 for ; Mon, 2 Sep 2019 09:53:17 -0400 (EDT) Received: from smtp15.mail.ru (smtp15.mail.ru [94.100.176.133]) (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 CF502285BF for ; Mon, 2 Sep 2019 09:53:16 -0400 (EDT) Date: Mon, 2 Sep 2019 16:53: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: <20190902135312.GA6335@tarantool.org> References: <67e721f4818f51861383cdf71cc88c63e45ab450.1563955619.git.imeevma@gmail.com> <20190727101643.GA10982@tarantool.org> <20190828133002.GA17433@tarantool.org> <20190829121930.GA51601@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190829121930.GA51601@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. New patch and my answers below. I didn't include diff between versions since patch is quite obvious and all I changed is created the helper function. On Thu, Aug 29, 2019 at 03:19:30PM +0300, Nikita Pettik wrote: > 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. > 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. > > > > + 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. > Done. > > 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. > > Added. Still, the error doesn't look right actually, but this problem I am going to solve for this errcode outside of this issue. New patch: >From 79932ca82d6fc3999ffedf707ac01bf2a17e9859 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 Since ARRAY and MAP cannot be converted to SCALAR type, this operation should throw an error. But when the error is raised in SQL, it is displayed in unreadable form. The reason for this is that the given array or map is not correctly converted to a string. This patch fixes the problem by converting ARRAY or MAP to their string representation. For example: box.execute('CREATE TABLE t1(i INT PRIMARY KEY, 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 SELECT * FROM t2;') Should return: - error: 'Type mismatch: can not convert [1, 2, 3] to scalar' Follow-up #4189 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) +{ + 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); assert(EIGHT_BYTE_ALIGNMENT(pMem)); + /* + * In case we have ARRAY/MAP we should save decoded value + * before clearing pMem->z. + */ + char *value = NULL; + if (sql_mem_has_msgpack_subtype(pMem)) { + const char *value_str = mp_str(pMem->z); + nByte = strlen(value_str) + 1; + value = region_alloc(&fiber()->gc, nByte); + memcpy(value, value_str, nByte); + } + if (sqlVdbeMemClearAndResize(pMem, nByte)) { return -1; } @@ -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.result b/test/sql/types.result index 1241ae4..5f0d181 100644 --- a/test/sql/types.result +++ b/test/sql/types.result @@ -2266,3 +2266,101 @@ 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);') +--- +- row_count: 1 +... +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}}) +--- +- [1, [1, 2, 3]] +... +box.execute('INSERT INTO t1(a) SELECT a FROM t2;') +--- +- null +- 'Type mismatch: can not convert [1, 2, 3] to scalar' +... +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}}) +--- +- [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;') +--- +- null +- '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' +... +long_array = {} +--- +... +for i = 1,120 do long_array[i] = i end +--- +... +s:replace({1, long_array}) +--- +- [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, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, + 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, + 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, + 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, + 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, + 118, 119, 120]] +... +box.execute('INSERT INTO t1(a) SELECT a FROM t2;') +--- +- null +- '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, 31, 32, 33, 34, + 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, + 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, + 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, + 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, + 112, 113, 114, 115, 116, 117, 11' +... +s:drop() +--- +... +format[2].type = 'map' +--- +... +s = box.schema.space.create('T2', {format=format}) +--- +... +i = s:create_index('ii') +--- +... +s:insert({1, {b = 1}}) +--- +- [1, {'b': 1}] +... +box.execute('INSERT INTO t1(a) SELECT a FROM t2;') +--- +- null +- 'Type mismatch: can not convert {"b": 1} to scalar' +... +s:drop() +--- +... +box.execute('DROP TABLE t1;') +--- +- row_count: 1 +... 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;') +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;')