From: Mergen Imeev <imeevma@tarantool.org>
To: korablev@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
Date: Mon, 2 Sep 2019 16:53:13 +0300 [thread overview]
Message-ID: <20190902135312.GA6335@tarantool.org> (raw)
In-Reply-To: <20190829121930.GA51601@tarantool.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 <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.
>
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 <imeevma@gmail.com>
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;')
next prev parent reply other threads:[~2019-09-02 13:53 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 [this message]
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=20190902135312.GA6335@tarantool.org \
--to=imeevma@tarantool.org \
--cc=korablev@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