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 2A0F227F64 for ; Thu, 22 Nov 2018 13:09:19 -0500 (EST) 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 QrrZjfsAqjHU for ; Thu, 22 Nov 2018 13:09:19 -0500 (EST) 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 B534627EC6 for ; Thu, 22 Nov 2018 13:09:18 -0500 (EST) From: Imeev Mergen Subject: [tarantool-patches] Re: [PATCH 2/3] sql: SELECT from system spaces returns unpacked msgpack. References: <9e22a12510a952d2378d8b926db266f4a426c332.1542635129.git.v.shpilevoy@tarantool.org> <5233E694-9416-4094-8E73-1F46B3205C91@tarantool.org> Message-ID: Date: Thu, 22 Nov 2018 21:09:16 +0300 MIME-Version: 1.0 In-Reply-To: <5233E694-9416-4094-8E73-1F46B3205C91@tarantool.org> Content-Type: multipart/alternative; boundary="------------78B21E2885E48EBD6FD61D96" Content-Language: en-US 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: "n.pettik" , tarantool-patches@freelists.org Cc: Vladislav Shpilevoy This is a multi-part message in MIME format. --------------78B21E2885E48EBD6FD61D96 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit 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 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') --------------78B21E2885E48EBD6FD61D96 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

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')

--------------78B21E2885E48EBD6FD61D96--