[tarantool-patches] [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[tarantool-patches] [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Kirill Shcherbatov]

Tarantool SQL used to return 'number' type in request metadata
for arithmetic operations even when only 'integer's were used.

This also fixes query planner optimisation bug:
SELECT a FROM t1 WHERE a+0 IN (SELECT a FROM t1);
used to open a new ephemeral table with OpenTEphemeral when
it is not required (introduced by 2b22b913).

Closes #4103
---
Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4103-invalid-type-in-operations
Issue: https://github.com/tarantool/tarantool/issues/4103

 src/box/sql/expr.c                   | 13 ++++++++++++-
 test/sql-tap/in3.test.lua            |  2 +-
 test/sql-tap/tkt-80e031a00f.test.lua |  8 ++++----
 test/sql/gh-3199-no-mem-leaks.result | 24 ++++++++++++------------
 test/sql/types.result                | 27 +++++++++++++++++++++++++++
 test/sql/types.test.lua              |  7 +++++++
 6 files changed, 63 insertions(+), 18 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index b3613d3ea..9b52e90f3 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -342,8 +342,19 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 enum field_type
 sql_type_result(enum field_type lhs, enum field_type rhs)
 {
-	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
+	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs)) {
+		if (lhs == FIELD_TYPE_NUMBER || rhs == FIELD_TYPE_NUMBER)
+			return FIELD_TYPE_NUMBER;
+		if (lhs == FIELD_TYPE_INTEGER || rhs == FIELD_TYPE_INTEGER)
+			return FIELD_TYPE_INTEGER;
+		/*
+		 * FIXME: FIELD_TYPE_UNSIGNED static type is not
+		 * allowed yet.
+		 */
+		assert(lhs == FIELD_TYPE_UNSIGNED ||
+		       rhs == FIELD_TYPE_UNSIGNED);
 		return FIELD_TYPE_NUMBER;
+	}
 	return FIELD_TYPE_SCALAR;
 }
 
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index a5e31f8a7..1ca6a6446 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -92,7 +92,7 @@ test:do_test(
         return exec_neph(" SELECT a FROM t1 WHERE a+0 IN (SELECT a FROM t1); ")
     end, {
         -- <in3-1.5>
-        1, 1, 3, 5
+        0, 1, 3, 5
         -- </in3-1.5>
     })
 
diff --git a/test/sql-tap/tkt-80e031a00f.test.lua b/test/sql-tap/tkt-80e031a00f.test.lua
index 7fd348632..3edea11ac 100755
--- a/test/sql-tap/tkt-80e031a00f.test.lua
+++ b/test/sql-tap/tkt-80e031a00f.test.lua
@@ -346,7 +346,7 @@ test:do_catchsql_test(
         SELECT 'hello' IN t1
     ]], {
         -- <tkt-80e031a00f.27>
-        1, 'Type mismatch: can not convert hello to number'
+        1, 'Type mismatch: can not convert hello to integer'
         -- </tkt-80e031a00f.27>
     })
 
@@ -356,7 +356,7 @@ test:do_catchsql_test(
         SELECT 'hello' NOT IN t1
     ]], {
         -- <tkt-80e031a00f.28>
-        1, 'Type mismatch: can not convert hello to number'
+        1, 'Type mismatch: can not convert hello to integer'
         -- </tkt-80e031a00f.28>
     })
 
@@ -386,7 +386,7 @@ test:do_catchsql_test(
         SELECT x'303132' IN t1
     ]], {
         -- <tkt-80e031a00f.31>
-        1, 'Type mismatch: can not convert 012 to number'
+        1, 'Type mismatch: can not convert 012 to integer'
         -- </tkt-80e031a00f.31>
     })
 
@@ -396,7 +396,7 @@ test:do_catchsql_test(
         SELECT x'303132' NOT IN t1
     ]], {
         -- <tkt-80e031a00f.32>
-        1, 'Type mismatch: can not convert 012 to number'
+        1, 'Type mismatch: can not convert 012 to integer'
         -- </tkt-80e031a00f.32>
     })
 
diff --git a/test/sql/gh-3199-no-mem-leaks.result b/test/sql/gh-3199-no-mem-leaks.result
index 22be46a82..e7ba1d29c 100644
--- a/test/sql/gh-3199-no-mem-leaks.result
+++ b/test/sql/gh-3199-no-mem-leaks.result
@@ -31,7 +31,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -48,7 +48,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -61,7 +61,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -74,7 +74,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -87,7 +87,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -114,7 +114,7 @@ box.execute('SELECT a, id + 2, b FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
@@ -133,7 +133,7 @@ box.execute('SELECT a, id + 2 * b, a FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2 * b
-    type: number
+    type: integer
   - name: A
     type: string
   rows:
@@ -148,7 +148,7 @@ box.execute('SELECT a, id + 2 * b, a FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2 * b
-    type: number
+    type: integer
   - name: A
     type: string
   rows:
@@ -163,7 +163,7 @@ box.execute('SELECT a, id + 2 * b, a FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2 * b
-    type: number
+    type: integer
   - name: A
     type: string
   rows:
@@ -182,7 +182,7 @@ box.execute('SELECT x, y + 3 * b, b FROM test2, test WHERE b = x')
   - name: X
     type: integer
   - name: y + 3 * b
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
@@ -195,7 +195,7 @@ box.execute('SELECT x, y + 3 * b, b FROM test2, test WHERE b = x')
   - name: X
     type: integer
   - name: y + 3 * b
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
@@ -208,7 +208,7 @@ box.execute('SELECT x, y + 3 * b, b FROM test2, test WHERE b = x')
   - name: X
     type: integer
   - name: y + 3 * b
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
diff --git a/test/sql/types.result b/test/sql/types.result
index de17bbb78..d55addab3 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -860,3 +860,30 @@ box.space.T:drop()
 box.space.T1:drop()
 ---
 ...
+--
+-- gh-4103: Invalid sum type
+--
+box.execute('SELECT 1 + 1;')
+---
+- metadata:
+  - name: 1 + 1
+    type: integer
+  rows:
+  - [2]
+...
+box.execute('SELECT 1 + 1.1;')
+---
+- metadata:
+  - name: 1 + 1.1
+    type: number
+  rows:
+  - [2.1]
+...
+box.execute('SELECT \'9223372036854\' + 1;')
+---
+- metadata:
+  - name: '''9223372036854'' + 1'
+    type: integer
+  rows:
+  - [9223372036855]
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 461635978..d777c8b39 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -205,3 +205,10 @@ box.execute("SELECT s FROM t1 WHERE s IN (true, 1, 'abcd')")
 
 box.space.T:drop()
 box.space.T1:drop()
+
+--
+-- gh-4103: Invalid sum type
+--
+box.execute('SELECT 1 + 1;')
+box.execute('SELECT 1 + 1.1;')
+box.execute('SELECT \'9223372036854\' + 1;')
-- 
2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[n.pettik]

> diff --git a/test/sql/types.result b/test/sql/types.result
> index de17bbb78..d55addab3 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -860,3 +860,30 @@ box.space.T:drop()
> box.space.T1:drop()
> ---
> ...
> +--
> +-- gh-4103: Invalid sum type

-> if resulting value of arithmetic operations is integers,
then make sure its type also integer (not number).

The rest is OK.

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Vladislav Shpilevoy]

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index b3613d3ea..9b52e90f3 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -342,8 +342,19 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
>  enum field_type
>  sql_type_result(enum field_type lhs, enum field_type rhs)
>  {
> -	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
> +	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs)) {
> +		if (lhs == FIELD_TYPE_NUMBER || rhs == FIELD_TYPE_NUMBER)
> +			return FIELD_TYPE_NUMBER;
> +		if (lhs == FIELD_TYPE_INTEGER || rhs == FIELD_TYPE_INTEGER)
> +			return FIELD_TYPE_INTEGER;
> +		/*
> +		 * FIXME: FIELD_TYPE_UNSIGNED static type is not
> +		 * allowed yet.
> +		 */
> +		assert(lhs == FIELD_TYPE_UNSIGNED ||
> +		       rhs == FIELD_TYPE_UNSIGNED);

How does it work? If it is not allowed, then lhs and rhs should not
be equal to FIELD_TYPE_UNSIGNED, and this assertion should fail, it is not?

(I did not test, just looked at the diff in the mailing list)

>  		return FIELD_TYPE_NUMBER;
> +	}
>  	return FIELD_TYPE_SCALAR;
>  }
>  

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[n.pettik]

> On 24 Apr 2019, at 22:56, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index b3613d3ea..9b52e90f3 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -342,8 +342,19 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
>> enum field_type
>> sql_type_result(enum field_type lhs, enum field_type rhs)
>> {
>> -	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
>> +	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs)) {
>> +		if (lhs == FIELD_TYPE_NUMBER || rhs == FIELD_TYPE_NUMBER)
>> +			return FIELD_TYPE_NUMBER;
>> +		if (lhs == FIELD_TYPE_INTEGER || rhs == FIELD_TYPE_INTEGER)
>> +			return FIELD_TYPE_INTEGER;
>> +		/*
>> +		 * FIXME: FIELD_TYPE_UNSIGNED static type is not
>> +		 * allowed yet.
>> +		 */
>> +		assert(lhs == FIELD_TYPE_UNSIGNED ||
>> +		       rhs == FIELD_TYPE_UNSIGNED);
> 
> How does it work? If it is not allowed, then lhs and rhs should not
> be equal to FIELD_TYPE_UNSIGNED, and this assertion should fail, it is not?
> 
> (I did not test, just looked at the diff in the mailing list)


Execution flaw hits this branch if both operands are numeric.
We consider only _INTEGER, _NUMBER and _UNSIGNED be
numeric types. If we hit this assertion, then both operands should
be _UNSIGNED (if I’m not mistaken, condition should be logical
AND not OR).

Surely, we still can create space and set format with unsigned types
from Lua, so strictly speaking UNSIGNED is allowed even now.
But we can’t set UNSIGNED as a type of column in SQL, and we don’t
set this type in meta. So in some sense it is not allowed.
Mb it is worth fixing comment. Or return _UNSIGNED instead
of _NUMBER in this case. I guess there will be no severe consequences.

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Kirill Shcherbatov]

> -> if resulting value of arithmetic operations is integers,
> then make sure its type also integer (not number).
Updated.

> Execution flaw hits this branch if both operands are numeric.
> We consider only _INTEGER, _NUMBER and _UNSIGNED be
> numeric types. If we hit this assertion, then both operands should
> be _UNSIGNED (if I’m not mistaken, condition should be logical
> AND not OR).
> 
> Surely, we still can create space and set format with unsigned types
> from Lua, so strictly speaking UNSIGNED is allowed even now.
> But we can’t set UNSIGNED as a type of column in SQL, and we don’t
> set this type in meta. So in some sense it is not allowed.
> Mb it is worth fixing comment. Or return _UNSIGNED instead
> of _NUMBER in this case. I guess there will be no severe consequences.

Let's remove this comment and return _UNSIGNED.

> The rest is OK.

================================================

Tarantool SQL used to return 'number' type in request metadata
for arithmetic operations even when only 'integer's were used.

This also fixes query planner optimisation bug:
SELECT a FROM t1 WHERE a+0 IN (SELECT a FROM t1);
used to open a new ephemeral table with OpenTEphemeral when
it is not required (introduced by 2b22b913).

Closes #4103
---
 src/box/sql/expr.c                   | 11 +++++++++--
 test/sql-tap/in3.test.lua            |  2 +-
 test/sql-tap/tkt-80e031a00f.test.lua |  8 ++++----
 test/sql/gh-3199-no-mem-leaks.result | 24 ++++++++++++------------
 test/sql/types.result                | 28 ++++++++++++++++++++++++++++
 test/sql/types.test.lua              |  8 ++++++++
 6 files changed, 62 insertions(+), 19 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index b3613d3ea..ba7eea59d 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -342,8 +342,15 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
 enum field_type
 sql_type_result(enum field_type lhs, enum field_type rhs)
 {
-	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
-		return FIELD_TYPE_NUMBER;
+	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs)) {
+		if (lhs == FIELD_TYPE_NUMBER || rhs == FIELD_TYPE_NUMBER)
+			return FIELD_TYPE_NUMBER;
+		if (lhs == FIELD_TYPE_INTEGER || rhs == FIELD_TYPE_INTEGER)
+			return FIELD_TYPE_INTEGER;
+		assert(lhs == FIELD_TYPE_UNSIGNED ||
+		       rhs == FIELD_TYPE_UNSIGNED);
+		return FIELD_TYPE_UNSIGNED;
+	}
 	return FIELD_TYPE_SCALAR;
 }
 
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index a5e31f8a7..1ca6a6446 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -92,7 +92,7 @@ test:do_test(
         return exec_neph(" SELECT a FROM t1 WHERE a+0 IN (SELECT a FROM t1); ")
     end, {
         -- <in3-1.5>
-        1, 1, 3, 5
+        0, 1, 3, 5
         -- </in3-1.5>
     })
 
diff --git a/test/sql-tap/tkt-80e031a00f.test.lua b/test/sql-tap/tkt-80e031a00f.test.lua
index 7fd348632..3edea11ac 100755
--- a/test/sql-tap/tkt-80e031a00f.test.lua
+++ b/test/sql-tap/tkt-80e031a00f.test.lua
@@ -346,7 +346,7 @@ test:do_catchsql_test(
         SELECT 'hello' IN t1
     ]], {
         -- <tkt-80e031a00f.27>
-        1, 'Type mismatch: can not convert hello to number'
+        1, 'Type mismatch: can not convert hello to integer'
         -- </tkt-80e031a00f.27>
     })
 
@@ -356,7 +356,7 @@ test:do_catchsql_test(
         SELECT 'hello' NOT IN t1
     ]], {
         -- <tkt-80e031a00f.28>
-        1, 'Type mismatch: can not convert hello to number'
+        1, 'Type mismatch: can not convert hello to integer'
         -- </tkt-80e031a00f.28>
     })
 
@@ -386,7 +386,7 @@ test:do_catchsql_test(
         SELECT x'303132' IN t1
     ]], {
         -- <tkt-80e031a00f.31>
-        1, 'Type mismatch: can not convert 012 to number'
+        1, 'Type mismatch: can not convert 012 to integer'
         -- </tkt-80e031a00f.31>
     })
 
@@ -396,7 +396,7 @@ test:do_catchsql_test(
         SELECT x'303132' NOT IN t1
     ]], {
         -- <tkt-80e031a00f.32>
-        1, 'Type mismatch: can not convert 012 to number'
+        1, 'Type mismatch: can not convert 012 to integer'
         -- </tkt-80e031a00f.32>
     })
 
diff --git a/test/sql/gh-3199-no-mem-leaks.result b/test/sql/gh-3199-no-mem-leaks.result
index 22be46a82..e7ba1d29c 100644
--- a/test/sql/gh-3199-no-mem-leaks.result
+++ b/test/sql/gh-3199-no-mem-leaks.result
@@ -31,7 +31,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -48,7 +48,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -61,7 +61,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -74,7 +74,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -87,7 +87,7 @@ box.execute('SELECT x, y, x + y FROM test ORDER BY y')
   - name: Y
     type: integer
   - name: x + y
-    type: number
+    type: integer
   rows:
   - [1, 1, 2]
   - [2, 2, 4]
@@ -114,7 +114,7 @@ box.execute('SELECT a, id + 2, b FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
@@ -133,7 +133,7 @@ box.execute('SELECT a, id + 2 * b, a FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2 * b
-    type: number
+    type: integer
   - name: A
     type: string
   rows:
@@ -148,7 +148,7 @@ box.execute('SELECT a, id + 2 * b, a FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2 * b
-    type: number
+    type: integer
   - name: A
     type: string
   rows:
@@ -163,7 +163,7 @@ box.execute('SELECT a, id + 2 * b, a FROM test2 WHERE b < id * 2 ORDER BY a ')
   - name: A
     type: string
   - name: id + 2 * b
-    type: number
+    type: integer
   - name: A
     type: string
   rows:
@@ -182,7 +182,7 @@ box.execute('SELECT x, y + 3 * b, b FROM test2, test WHERE b = x')
   - name: X
     type: integer
   - name: y + 3 * b
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
@@ -195,7 +195,7 @@ box.execute('SELECT x, y + 3 * b, b FROM test2, test WHERE b = x')
   - name: X
     type: integer
   - name: y + 3 * b
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
@@ -208,7 +208,7 @@ box.execute('SELECT x, y + 3 * b, b FROM test2, test WHERE b = x')
   - name: X
     type: integer
   - name: y + 3 * b
-    type: number
+    type: integer
   - name: B
     type: integer
   rows:
diff --git a/test/sql/types.result b/test/sql/types.result
index de17bbb78..8f442dc7d 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -860,3 +860,31 @@ box.space.T:drop()
 box.space.T1:drop()
 ---
 ...
+--
+-- gh-4103: If resulting value of arithmetic operations is
+-- integers, then make sure its type also integer (not number).
+--
+box.execute('SELECT 1 + 1;')
+---
+- metadata:
+  - name: 1 + 1
+    type: integer
+  rows:
+  - [2]
+...
+box.execute('SELECT 1 + 1.1;')
+---
+- metadata:
+  - name: 1 + 1.1
+    type: number
+  rows:
+  - [2.1]
+...
+box.execute('SELECT \'9223372036854\' + 1;')
+---
+- metadata:
+  - name: '''9223372036854'' + 1'
+    type: integer
+  rows:
+  - [9223372036855]
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index 461635978..48c9bde10 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -205,3 +205,11 @@ box.execute("SELECT s FROM t1 WHERE s IN (true, 1, 'abcd')")
 
 box.space.T:drop()
 box.space.T1:drop()
+
+--
+-- gh-4103: If resulting value of arithmetic operations is
+-- integers, then make sure its type also integer (not number).
+--
+box.execute('SELECT 1 + 1;')
+box.execute('SELECT 1 + 1.1;')
+box.execute('SELECT \'9223372036854\' + 1;')
-- 
2.21.0

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Kirill Yukhin]

Hello,

On 24 Apr 18:36, Kirill Shcherbatov wrote:
> Tarantool SQL used to return 'number' type in request metadata
> for arithmetic operations even when only 'integer's were used.
> 
> This also fixes query planner optimisation bug:
> SELECT a FROM t1 WHERE a+0 IN (SELECT a FROM t1);
> used to open a new ephemeral table with OpenTEphemeral when
> it is not required (introduced by 2b22b913).
> 
> Closes #4103
> ---
> Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4103-invalid-type-in-operations
> Issue: https://github.com/tarantool/tarantool/issues/4103

I've checked your patch into master.

--
Regards, Kirill Yukhin

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Konstantin Osipov]

* n.pettik <korablev@tarantool.org> [19/04/25 09:18]:
> Surely, we still can create space and set format with unsigned types
> from Lua, so strictly speaking UNSIGNED is allowed even now.
> But we can’t set UNSIGNED as a type of column in SQL, and we don’t
> set this type in meta. So in some sense it is not allowed.
> Mb it is worth fixing comment. Or return _UNSIGNED instead
> of _NUMBER in this case. I guess there will be no severe consequences.

SQL *is* server. Please begin by having a comment containing a
table for integer type arithmetics, something like this:

  Plus operation:
  --------------
  
               integer   float

  integer      integer   float
  float        float     float

It's best to implement such table in the source code too, and not
have complicated branching.

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

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Konstantin Osipov]

* Konstantin Osipov <kostja.osipov@gmail.com> [19/04/25 13:49]:
> * n.pettik <korablev@tarantool.org> [19/04/25 09:18]:
> > Surely, we still can create space and set format with unsigned types
> > from Lua, so strictly speaking UNSIGNED is allowed even now.
> > But we can’t set UNSIGNED as a type of column in SQL, and we don’t
> > set this type in meta. So in some sense it is not allowed.
> > Mb it is worth fixing comment. Or return _UNSIGNED instead
> > of _NUMBER in this case. I guess there will be no severe consequences.
> 
> SQL *is* server. Please begin by having a comment containing a
> table for integer type arithmetics, something like this:
> 
>   Plus operation:
>   --------------
>   
>                integer   float
> 
>   integer      integer   float
>   float        float     float
> 
> It's best to implement such table in the source code too, and not
> have complicated branching.

Re UNSIGNED type itself:

once you have a proper table for type arithmetics, it will be easy
to understand what the result of unsigned + unsigned should be.
Most likely it should be unsigned, not a number.

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

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Kirill Shcherbatov]

>> SQL *is* server. Please begin by having a comment containing a
>> table for integer type arithmetics, something like this:
>>
>>   Plus operation:
>>   --------------
>>   
>>                integer   float
>>
>>   integer      integer   float
>>   float        float     float
>>
>> It's best to implement such table in the source code too, and not
>> have complicated branching.
> 
> Re UNSIGNED type itself:
> 
> once you have a proper table for type arithmetics, it will be easy
> to understand what the result of unsigned + unsigned should be.
> Most likely it should be unsigned, not a number.

Exactly this logic is already on master branch (but only with few sequential ifs, not a table).

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Konstantin Osipov]

* Kirill Shcherbatov <kshcherbatov@tarantool.org> [19/04/25 14:12]:
> Exactly this logic is already on master branch (but only with few sequential ifs, not a table).

Please do a table. You already have a branching hell, and UNSIGNED
arithmetics is raising questions. In particular, I disagree that unsigned +
unsigned should give you number - it should give you unsigned. 
-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[Kirill Shcherbatov]

> Please do a table. You already have a branching hell, and UNSIGNED
> arithmetics is raising questions. In particular, 

> I disagree that unsigned + unsigned should give you number - it should give you unsigned. 
It is not so, look (master, ff8462554d33d971e6d6df3c6e8913785e7d9d61):

-       if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
-               return FIELD_TYPE_NUMBER;
+       if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs)) {
+               if (lhs == FIELD_TYPE_NUMBER || rhs == FIELD_TYPE_NUMBER)
+                       return FIELD_TYPE_NUMBER;
+               if (lhs == FIELD_TYPE_INTEGER || rhs == FIELD_TYPE_INTEGER)
+                       return FIELD_TYPE_INTEGER;
+               assert(lhs == FIELD_TYPE_UNSIGNED ||
+                      rhs == FIELD_TYPE_UNSIGNED);
+               return FIELD_TYPE_UNSIGNED;
+       }