[tarantool-patches] [PATCH 0/2] SQL types fixes

[tarantool-patches] [PATCH 0/2] SQL types fixes

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/sql-types-fixes
No related issue

First patch fixes names send as column meta-information. After
BLOB type was substituted with SCALAR, it can be confusing saying
that column has type BLOB. Moreover, other string represantations
have been replaced with exact names of NoSQL types (i.e. STRING,
NUMER etc).

Second patch fixes wrong type calculation for returning value for
function: it takes place during name resolution, and its second
promotion during byte-code generation is redundant (and in some
cases is wrong).

Nikita Pettik (2):
  sql: make type in column-meta be consistent with NoSQL names
  sql: don't change type of function's retval after codegen

 src/box/sql/expr.c                           |  11 ---
 src/box/sql/select.c                         |  31 +-------
 test/sql/errinj.result                       |   2 +-
 test/sql/gh-2362-select-access-rights.result |   4 +-
 test/sql/iproto.result                       | 109 ++++++++++++++++++---------
 test/sql/iproto.test.lua                     |  15 ++++
 6 files changed, 97 insertions(+), 75 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/2] sql: make type in column-meta be consistent with NoSQL names

[Nikita Pettik]

Column meta-information which is sent alongside execution result via
IProto protocol, contains string representation of column type.
In some cases, name of type is different from actual type of field. For
instance, if column has type SCALAR, string representation in
meta-information will be "BLOB"; for NUMBER NoSQL type - it will be
"NUMERIC"; for STRING - "TEXT". Instead of this mess, let's always return
exact name of underlying NoSQL type.
---
 src/box/sql/select.c                         | 31 ++-----------
 test/sql/errinj.result                       |  2 +-
 test/sql/gh-2362-select-access-rights.result |  4 +-
 test/sql/iproto.result                       | 68 ++++++++++++++--------------
 4 files changed, 41 insertions(+), 64 deletions(-)

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 782da1f7c..8e8125195 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1739,33 +1739,10 @@ generateColumnNames(Parse * pParse,	/* Parser context */
 		p = pEList->a[i].pExpr;
 		if (NEVER(p == 0))
 			continue;
-		switch (p->type) {
-		case FIELD_TYPE_INTEGER:
-			sqlVdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER",
-					      SQL_TRANSIENT);
-			break;
-		case FIELD_TYPE_NUMBER:
-			sqlVdbeSetColName(v, i, COLNAME_DECLTYPE, "NUMERIC",
-					      SQL_TRANSIENT);
-			break;
-		case FIELD_TYPE_STRING:
-			sqlVdbeSetColName(v, i, COLNAME_DECLTYPE, "TEXT",
-					      SQL_TRANSIENT);
-			break;
-		case FIELD_TYPE_SCALAR:
-			sqlVdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB",
-					      SQL_TRANSIENT);
-			break;
-		case FIELD_TYPE_BOOLEAN:
-			if (p->op == TK_VARIABLE)
-				var_pos[var_count++] = i;
-			sqlVdbeSetColName(v, i, COLNAME_DECLTYPE, "BOOLEAN",
-					      SQL_TRANSIENT);
-			break;
-		default:
-			sqlVdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN",
-					      SQL_TRANSIENT);
-		}
+		if (p->op == TK_VARIABLE)
+			var_pos[var_count++] = i;
+		sqlVdbeSetColName(v, i, COLNAME_DECLTYPE,
+				  field_type_strs[p->type], SQL_TRANSIENT);
 		if (pEList->a[i].zName) {
 			char *zName = pEList->a[i].zName;
 			sqlVdbeSetColName(v, i, COLNAME_NAME, zName,
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index 0f6075b13..6763faf63 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -115,7 +115,7 @@ select_res
 ---
 - metadata:
   - name: '1'
-    type: INTEGER
+    type: integer
   rows:
   - [1]
 ...
diff --git a/test/sql/gh-2362-select-access-rights.result b/test/sql/gh-2362-select-access-rights.result
index 0e5b9bf56..39b38bcf7 100644
--- a/test/sql/gh-2362-select-access-rights.result
+++ b/test/sql/gh-2362-select-access-rights.result
@@ -32,9 +32,9 @@ c:execute("SELECT * FROM t1;")
 ---
 - metadata:
   - name: S1
-    type: INTEGER
+    type: integer
   - name: S2
-    type: INTEGER
+    type: integer
   rows:
   - [1, 1]
 ...
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index da7b40f22..3a77c8e93 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -58,11 +58,11 @@ ret
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: NUMERIC
+    type: number
   - name: B
-    type: TEXT
+    type: string
   rows:
   - [1, 2, '3']
   - [4, 5, '6']
@@ -103,7 +103,7 @@ cn:execute('select id as identifier from test where a = 5;')
 ---
 - metadata:
   - name: IDENTIFIER
-    type: INTEGER
+    type: integer
   rows: []
 ...
 -- netbox API errors.
@@ -131,11 +131,11 @@ cn:execute('select * from test where id = ?', {1})
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: NUMERIC
+    type: number
   - name: B
-    type: TEXT
+    type: string
   rows:
   - [1, 2, '3']
 ...
@@ -143,11 +143,11 @@ cn:execute('select * from test limit ?', {2})
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: NUMERIC
+    type: number
   - name: B
-    type: TEXT
+    type: string
   rows:
   - [1, 2, '3']
   - [7, 8.5, '9']
@@ -171,11 +171,11 @@ cn:execute('select * from test limit 1 offset ?', {2})
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: NUMERIC
+    type: number
   - name: B
-    type: TEXT
+    type: string
   rows:
   - [10, 11, null]
 ...
@@ -210,11 +210,11 @@ cn:execute('select * from test where id = :value', parameters)
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: NUMERIC
+    type: number
   - name: B
-    type: TEXT
+    type: string
   rows:
   - [1, 2, '3']
 ...
@@ -306,7 +306,7 @@ cn:execute('select :value3, ?, :value1, ?, ?, @value2, ?, :value3', parameters)
   - name: '@value2'
     type: INTEGER
   - name: '?'
-    type: BOOLEAN
+    type: boolean
   - name: :value3
     type: INTEGER
   rows:
@@ -436,13 +436,13 @@ cn:execute('select * from test2')
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: INTEGER
+    type: integer
   - name: B
-    type: INTEGER
+    type: integer
   - name: C
-    type: INTEGER
+    type: integer
   rows:
   - [1, 1, 1, 1]
 ...
@@ -602,11 +602,11 @@ cn:execute('select * from test where id = :1', {1})
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: NUMERIC
+    type: number
   - name: B
-    type: TEXT
+    type: string
   rows:
   - [1, 2, '3']
 ...
@@ -620,11 +620,11 @@ res = cn:execute('select * from test')
 res.metadata
 ---
 - - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: NUMERIC
+    type: number
   - name: B
-    type: TEXT
+    type: string
 ...
 box.sql.execute('drop table test')
 ---
@@ -672,11 +672,11 @@ future4:wait_result()
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: INTEGER
+    type: integer
   - name: B
-    type: INTEGER
+    type: integer
   rows:
   - [1, 1, 1]
   - [2, 2, 2]
@@ -731,9 +731,9 @@ cn:execute('select * from test')
 ---
 - metadata:
   - name: ID
-    type: INTEGER
+    type: integer
   - name: A
-    type: INTEGER
+    type: integer
   rows:
   - [1, 11]
   - [2, 2]
@@ -850,9 +850,9 @@ cn:execute('select * from "test"')
 ---
 - metadata:
   - name: id
-    type: INTEGER
+    type: integer
   - name: x
-    type: UNKNOWN
+    type: any
   rows:
   - [1, [1, 2, 3]]
   - [2, {'a': 3}]
-- 
2.15.1

[tarantool-patches] [PATCH 2/2] sql: don't change type of function's retval after codegen

[Nikita Pettik]

Proper type of function's returning value is set during names resolution
(i.e. promotion from struct FuncDef to struct Expr, see
resolveExprStep()). Accidentally, it was set again during byte-code
generation for VDBE. What is more, in some cases it was set to a wrong
type. For instance, built-in function randomblob() returns value to be
encoded as MP_BIN, so its returning type is SCALAR. However, it was
reset to INTEGER (as its first argument). This patch simply removes this
second type promotion.
---
 src/box/sql/expr.c       | 11 -----------
 test/sql/iproto.result   | 41 +++++++++++++++++++++++++++++++++++++++++
 test/sql/iproto.test.lua | 15 +++++++++++++++
 3 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index a75f23756..8efd0631a 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4000,17 +4000,6 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				sql_parser_error(pParse);
 				break;
 			}
-
-			if (pDef->ret_type != FIELD_TYPE_SCALAR) {
-				pExpr->type = pDef->ret_type;
-			} else {
-				/*
-				 * Otherwise, use first arg as
-				 * expression type.
-				 */
-				if (pFarg && pFarg->nExpr > 0)
-					pExpr->type = pFarg->a[0].pExpr->type;
-			}
 			/* Attempt a direct implementation of the built-in COALESCE() and
 			 * IFNULL() functions.  This avoids unnecessary evaluation of
 			 * arguments past the first non-NULL argument.
diff --git a/test/sql/iproto.result b/test/sql/iproto.result
index 3a77c8e93..26d55b60c 100644
--- a/test/sql/iproto.result
+++ b/test/sql/iproto.result
@@ -942,6 +942,47 @@ res.metadata
   - name: detail
     type: TEXT
 ...
+-- Make sure that built-in functions have a right returning type.
+--
+cn:execute("SELECT zeroblob(1);")
+---
+- metadata:
+  - name: zeroblob(1)
+    type: scalar
+  rows:
+  - ["\0"]
+...
+-- randomblob() returns different results each time, so check only
+-- type in meta.
+--
+res = cn:execute("SELECT randomblob(1);")
+---
+...
+res.metadata
+---
+- - name: randomblob(1)
+    type: scalar
+...
+-- Type set during compilation stage, and since min/max are accept
+-- arguments of all scalar type, we can't say nothing more than
+-- SCALAR.
+--
+cn:execute("SELECT min(1, 2, 3);")
+---
+- metadata:
+  - name: min(1, 2, 3)
+    type: scalar
+  rows:
+  - [1]
+...
+cn:execute("SELECT max(1, 2, 3);")
+---
+- metadata:
+  - name: max(1, 2, 3)
+    type: scalar
+  rows:
+  - [3]
+...
 cn:close()
 ---
 ...
diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua
index fbdc5a2ae..89d04b638 100644
--- a/test/sql/iproto.test.lua
+++ b/test/sql/iproto.test.lua
@@ -289,6 +289,21 @@ res.metadata
 res = cn:execute("EXPLAIN QUERY PLAN SELECT COUNT(*) FROM t1")
 res.metadata
 
+-- Make sure that built-in functions have a right returning type.
+--
+cn:execute("SELECT zeroblob(1);")
+-- randomblob() returns different results each time, so check only
+-- type in meta.
+--
+res = cn:execute("SELECT randomblob(1);")
+res.metadata
+-- Type set during compilation stage, and since min/max are accept
+-- arguments of all scalar type, we can't say nothing more than
+-- SCALAR.
+--
+cn:execute("SELECT min(1, 2, 3);")
+cn:execute("SELECT max(1, 2, 3);")
+
 cn:close()
 box.sql.execute('DROP TABLE t1')
 
-- 
2.15.1

[tarantool-patches] Re: [PATCH 1/2] sql: make type in column-meta be consistent with NoSQL names

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 2 comments below.

On 11/03/2019 21:10, Nikita Pettik wrote:
> Column meta-information which is sent alongside execution result via
> IProto protocol, contains string representation of column type.
> In some cases, name of type is different from actual type of field. For
> instance, if column has type SCALAR, string representation in
> meta-information will be "BLOB"; for NUMBER NoSQL type - it will be
> "NUMERIC"; for STRING - "TEXT". Instead of this mess, let's always return
> exact name of underlying NoSQL type.

1. As I know, Kostja deliberately kept SQL names different from NoSQL names,
because SQL has strict standard typing != Tarantool one's. Please, ask him
if your renaming is ok.

> ---
>   src/box/sql/select.c                         | 31 ++-----------
>   test/sql/errinj.result                       |  2 +-
>   test/sql/gh-2362-select-access-rights.result |  4 +-
>   test/sql/iproto.result                       | 68 ++++++++++++++--------------
>   4 files changed, 41 insertions(+), 64 deletions(-)
> 
> diff --git a/test/sql/errinj.result b/test/sql/errinj.result
> index 0f6075b13..6763faf63 100644
> --- a/test/sql/errinj.result
> +++ b/test/sql/errinj.result
> @@ -115,7 +115,7 @@ select_res
>   ---
>   - metadata:
>     - name: '1'
> -    type: INTEGER
> +    type: integer

2. Not sure, if lowcase is ok. As I know, all SQL identifiers, keywords
should be stored and returned in uppercase (according to what I heard
from the one, who did the patch for uppercasing everything). Please,
consult Kostja, and check out other vendors for that.

[tarantool-patches] Re: [PATCH 2/2] sql: don't change type of function's retval after codegen

[Vladislav Shpilevoy]

Thanks for the patch!

On 11/03/2019 21:10, Nikita Pettik wrote:
> Proper type of function's returning value is set during names resolution
> (i.e. promotion from struct FuncDef to struct Expr, see
> resolveExprStep()). Accidentally, it was set again during byte-code
> generation for VDBE. What is more, in some cases it was set to a wrong
> type. For instance, built-in function randomblob() returns value to be
> encoded as MP_BIN, so its returning type is SCALAR. However, it was
> reset to INTEGER (as its first argument). This patch simply removes this
> second type promotion.
> ---
>   src/box/sql/expr.c       | 11 -----------
>   test/sql/iproto.result   | 41 +++++++++++++++++++++++++++++++++++++++++
>   test/sql/iproto.test.lua | 15 +++++++++++++++
>   3 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
> index 3a77c8e93..26d55b60c 100644
> --- a/test/sql/iproto.result
> +++ b/test/sql/iproto.result
> +-- Type set during compilation stage, and since min/max are accept
> +-- arguments of all scalar type, we can't say nothing more than
> +-- SCALAR.
> +--
> +cn:execute("SELECT min(1, 2, 3);")
> +---
> +- metadata:
> +  - name: min(1, 2, 3)
> +    type: scalar

This is the case, when a first argument type would be ok. As I
understand, it is illegal to calculate min of multiple different
types, so its rettype should be equal to its first argument. It
is not?

> +  rows:
> +  - [1]
> +...

[tarantool-patches] Re: [PATCH 2/2] sql: don't change type of function's retval after codegen

[n.pettik]

> On 11/03/2019 21:10, Nikita Pettik wrote:
>> Proper type of function's returning value is set during names resolution
>> (i.e. promotion from struct FuncDef to struct Expr, see
>> resolveExprStep()). Accidentally, it was set again during byte-code
>> generation for VDBE. What is more, in some cases it was set to a wrong
>> type. For instance, built-in function randomblob() returns value to be
>> encoded as MP_BIN, so its returning type is SCALAR. However, it was
>> reset to INTEGER (as its first argument). This patch simply removes this
>> second type promotion.
>> ---
>>  src/box/sql/expr.c       | 11 -----------
>>  test/sql/iproto.result   | 41 +++++++++++++++++++++++++++++++++++++++++
>>  test/sql/iproto.test.lua | 15 +++++++++++++++
>>  3 files changed, 56 insertions(+), 11 deletions(-)
>> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>> index 3a77c8e93..26d55b60c 100644
>> --- a/test/sql/iproto.result
>> +++ b/test/sql/iproto.result
>> +-- Type set during compilation stage, and since min/max are accept
>> +-- arguments of all scalar type, we can't say nothing more than
>> +-- SCALAR.
>> +--
>> +cn:execute("SELECT min(1, 2, 3);")
>> +---
>> +- metadata:
>> +  - name: min(1, 2, 3)
>> +    type: scalar
> 
> This is the case, when a first argument type would be ok. As I
> understand, it is illegal to calculate min of multiple different
> types, so its rettype should be equal to its first argument. It
> is not?

Unfortunately, it is legal:

tarantool> SELECT min('abc', 1);
---
- - [1]
…

It is to be resolved in scope of this issue:
https://github.com/tarantool/tarantool/issues/4032

[tarantool-patches] Re: [PATCH 1/2] sql: make type in column-meta be consistent with NoSQL names

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/03/11 21:10]:
> Column meta-information which is sent alongside execution result via
> IProto protocol, contains string representation of column type.
> In some cases, name of type is different from actual type of field. For
> instance, if column has type SCALAR, string representation in
> meta-information will be "BLOB"; for NUMBER NoSQL type - it will be
> "NUMERIC"; for STRING - "TEXT". Instead of this mess, let's always return
> exact name of underlying NoSQL type.

I'm ok with this patch.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH 2/2] sql: don't change type of function's retval after codegen

[Vladislav Shpilevoy]

On 21/03/2019 15:51, n.pettik wrote:
> 
>> On 11/03/2019 21:10, Nikita Pettik wrote:
>>> Proper type of function's returning value is set during names resolution
>>> (i.e. promotion from struct FuncDef to struct Expr, see
>>> resolveExprStep()). Accidentally, it was set again during byte-code
>>> generation for VDBE. What is more, in some cases it was set to a wrong
>>> type. For instance, built-in function randomblob() returns value to be
>>> encoded as MP_BIN, so its returning type is SCALAR. However, it was
>>> reset to INTEGER (as its first argument). This patch simply removes this
>>> second type promotion.
>>> ---
>>>   src/box/sql/expr.c       | 11 -----------
>>>   test/sql/iproto.result   | 41 +++++++++++++++++++++++++++++++++++++++++
>>>   test/sql/iproto.test.lua | 15 +++++++++++++++
>>>   3 files changed, 56 insertions(+), 11 deletions(-)
>>> diff --git a/test/sql/iproto.result b/test/sql/iproto.result
>>> index 3a77c8e93..26d55b60c 100644
>>> --- a/test/sql/iproto.result
>>> +++ b/test/sql/iproto.result
>>> +-- Type set during compilation stage, and since min/max are accept
>>> +-- arguments of all scalar type, we can't say nothing more than
>>> +-- SCALAR.
>>> +--
>>> +cn:execute("SELECT min(1, 2, 3);")
>>> +---
>>> +- metadata:
>>> +  - name: min(1, 2, 3)
>>> +    type: scalar
>>
>> This is the case, when a first argument type would be ok. As I
>> understand, it is illegal to calculate min of multiple different
>> types, so its rettype should be equal to its first argument. It
>> is not?
> 
> Unfortunately, it is legal:
> 
> tarantool> SELECT min('abc', 1);
> ---
> - - [1]
> …
> 
> It is to be resolved in scope of this issue:
> https://github.com/tarantool/tarantool/issues/4032
> 
> 

Thanks for the patchset and the explanations! As I see, Kostja
is ok with your type name changes. Technically the patch LGTM.

[tarantool-patches] Re: [PATCH 0/2] SQL types fixes

[Kirill Yukhin]

Hello,

On 11 Mar 21:10, Nikita Pettik wrote:
> Branch: https://github.com/tarantool/tarantool/tree/np/sql-types-fixes
> No related issue
> 
> First patch fixes names send as column meta-information. After
> BLOB type was substituted with SCALAR, it can be confusing saying
> that column has type BLOB. Moreover, other string represantations
> have been replaced with exact names of NoSQL types (i.e. STRING,
> NUMER etc).
> 
> Second patch fixes wrong type calculation for returning value for
> function: it takes place during name resolution, and its second
> promotion during byte-code generation is redundant (and in some
> cases is wrong).
> 
> Nikita Pettik (2):
>   sql: make type in column-meta be consistent with NoSQL names
>   sql: don't change type of function's retval after codegen

I've checked the patchset into master and 2.1 branch.

--
Regards, Kirill Yukhin