[tarantool-patches] Re: [PATCH 2/3] sql: SELECT from system spaces returns unpacked msgpack.

[Imeev Mergen <imeevma@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 6460 bytes --]

Hi! Thank you for review! Diff between versions and new patch
below.

On 11/19/18 8:27 PM, n.pettik wrote:
> Please, don’t state problem in commit message. Instead, it would be
> better to indicate fix of the problem. I would call it like:
>
> sql: decode mgspack after SELECT from system spaces
>
> Otherwise it looks like you are introducing new bug :)
>
> Also, your patch affects not only system spaces.
> For example:
>
> box.schema.space.create('t’)
> box.space.t:format({{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}})
> box.space.t:create_index('i1', {parts = {1, 'int'}})
> box.space.t:insert({1, {1,2,3}})
> box.space.t:insert({2, {a = 3}})
>
> Before your patch:
>
> tarantool> cn:execute('select * from \"t\"')
> ---
> - metadata:
>    - name: id
>      type: UNKNOWN
>    - name: x
>      type: UNKNOWN
>    rows:
>    - [1, !!binary kwECAw==]
>    - [2, !!binary gaFhAw==]
> ...
>
> After:
>
> tarantool> cn:execute('select * from \"t\"')
> ---
> - metadata:
>    - name: id
>      type: UNKNOWN
>    - name: x
>      type: UNKNOWN
>    rows:
>    - [1, [1, 2, 3]]
>    - [2, {'a': 3}]
> …
>
> I guess more precise to say sort of:
>
> sql: decode ARRAY and MAP types after SELECT
>
> Patch itself is OK, but I would rework test in order to avoid
> using system spaces. It is up to you.
>
*Diff between versions:*

diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index b866140..6c50781 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -811,14 +811,37 @@ cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')
  ---
  ...
--- SELECT from system spaces returns unpacked msgpack.
-res = cn:execute('select "format" from "_space" limit 1;')
+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
  ---
  ...
-res.rows
+s = box.schema.space.create('test', {format=format})
+---
+...
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+s:insert({2, {a = 3}})
+---
+- [2, {'a': 3}]
+...
+cn:execute('select * from "test"')
+---
+- metadata:
+  - name: id
+    type: UNKNOWN
+  - name: x
+    type: UNKNOWN
+  rows:
+  - [1, [1, 2, 3]]
+  - [2, {'a': 3}]
+...
+s:drop()
  ---
-- - [[{'name': 'space_id', 'type': 'unsigned'}, {'name': 'lsn', 'type': 
'unsigned'},
-      {'name': 'tuple', 'type': 'array'}]]
  ...
  cn:close()
  ---
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 6299814..e12decd 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -263,9 +263,14 @@ box.sql.execute('CREATE TABLE test (id INT PRIMARY 
KEY)')
  cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')

--- SELECT from system spaces returns unpacked msgpack.
-res = cn:execute('select "format" from "_space" limit 1;')
-res.rows
+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
+s = box.schema.space.create('test', {format=format})
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+s:insert({1, {1,2,3}})
+s:insert({2, {a = 3}})
+cn:execute('select * from "test"')
+s:drop()

  cn:close()

*New patch:*

commit 8482e0c0bc1fa44faa6abf7b7b8a27b77e91db92
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Fri Nov 16 14:23:39 2018 +0300

     sql: decode ARRAY and MAP types after SELECT

     Before this patch MSGPACK received using SELECT statement through
     net.box was unpacked. Fixed in this patch.

diff --git a/src/box/execute.c b/src/box/execute.c
index cd809f8..fb3e08b 100644
--- a/src/box/execute.c
+++ b/src/box/execute.c
@@ -351,13 +351,21 @@ sql_column_to_messagepack(struct sqlite3_stmt 
*stmt, int i,
      }
      case SQLITE_BLOB: {
          uint32_t len = sqlite3_column_bytes(stmt, i);
-        size = mp_sizeof_bin(len);
-        char *pos = (char *) region_alloc(region, size);
-        if (pos == NULL)
-            goto oom;
-        const char *s;
-        s = (const char *)sqlite3_column_blob(stmt, i);
-        mp_encode_bin(pos, s, len);
+        const char *s =
+            (const char *)sqlite3_column_blob(stmt, i);
+        if (sql_column_subtype(stmt, i) == SQL_SUBTYPE_MSGPACK) {
+            size = len;
+            char *pos = (char *)region_alloc(region, size);
+            if (pos == NULL)
+                goto oom;
+            memcpy(pos, s, len);
+        } else {
+            size = mp_sizeof_bin(len);
+            char *pos = (char *)region_alloc(region, size);
+            if (pos == NULL)
+                goto oom;
+            mp_encode_bin(pos, s, len);
+        }
          break;
      }
      case SQLITE_NULL: {
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index f229427..6c50781 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -811,6 +811,38 @@ cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')
  ---
  ...
+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
+---
+...
+s = box.schema.space.create('test', {format=format})
+---
+...
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+s:insert({2, {a = 3}})
+---
+- [2, {'a': 3}]
+...
+cn:execute('select * from "test"')
+---
+- metadata:
+  - name: id
+    type: UNKNOWN
+  - name: x
+    type: UNKNOWN
+  rows:
+  - [1, [1, 2, 3]]
+  - [2, {'a': 3}]
+...
+s:drop()
+---
+...
  cn:close()
  ---
  ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index 9bcc7ef..e12decd 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -263,6 +263,15 @@ box.sql.execute('CREATE TABLE test (id INT PRIMARY 
KEY)')
  cn:execute("PRAGMA TABLE_INFO(test);")
  box.sql.execute('DROP TABLE test')

+-- SELECT returns unpacked msgpack.
+format = {{name = 'id', type = 'integer'}, {name = 'x', type = 'any'}}
+s = box.schema.space.create('test', {format=format})
+i1 = s:create_index('i1', {parts = {1, 'int'}})
+s:insert({1, {1,2,3}})
+s:insert({2, {a = 3}})
+cn:execute('select * from "test"')
+s:drop()
+
  cn:close()

  box.schema.user.revoke('guest', 'read,write,execute', 'universe')


[-- Attachment #2: Type: text/html, Size: 8661 bytes --]