[Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[imeevma]

This patch makes number to be union of UNSIGNED, INTEGER and
DOUBLE numeric types.

Closes #4233
Closes #4463
---
https://github.com/tarantool/tarantool/issues/4233
https://github.com/tarantool/tarantool/issues/4463
https://github.com/tarantool/tarantool/tree/imeevma/gh-4233-fix-number-field-type-in-sql

 src/box/sql/vdbe.c                   |  15 +----
 src/box/sql/vdbeInt.h                |   1 -
 src/box/sql/vdbemem.c                |  44 ++-------------
 test/sql-tap/cast.test.lua           |  32 +++++------
 test/sql-tap/e_select1.test.lua      |   2 +-
 test/sql-tap/numcast.test.lua        | 105 ++++++++++++++++++++++++++++++++++-
 test/sql-tap/select3.test.lua        |   2 +-
 test/sql-tap/sort.test.lua           |  12 ++--
 test/sql-tap/tkt-91e2e8ba6f.test.lua |  12 ++--
 test/sql/integer-overflow.result     |   2 +-
 test/sql/types.result                |   2 +-
 11 files changed, 142 insertions(+), 87 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index eab74db..620d74e 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1566,7 +1566,6 @@ case OP_Subtract:              /* same as TK_MINUS, in1, in2, out3 */
 case OP_Multiply:              /* same as TK_STAR, in1, in2, out3 */
 case OP_Divide:                /* same as TK_SLASH, in1, in2, out3 */
 case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
-	char bIntint;   /* Started out as two integer operands */
 	u32 flags;      /* Combined MEM_* flags from both inputs */
 	u16 type1;      /* Numeric type of left operand */
 	u16 type2;      /* Numeric type of right operand */
@@ -1589,7 +1588,6 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 		bool is_lhs_neg = pIn1->flags & MEM_Int;
 		bool is_rhs_neg = pIn2->flags & MEM_Int;
 		bool is_res_neg;
-		bIntint = 1;
 		switch( pOp->opcode) {
 		case OP_Add: {
 			if (sql_add_int(iA, is_lhs_neg, iB, is_rhs_neg,
@@ -1629,7 +1627,6 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 		}
 		mem_set_int(pOut, iB, is_res_neg);
 	} else {
-		bIntint = 0;
 		if (sqlVdbeRealValue(pIn1, &rA) != 0) {
 			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
 				 sql_value_to_diag_str(pIn1), "numeric");
@@ -1640,6 +1637,7 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 				 sql_value_to_diag_str(pIn2), "numeric");
 			goto abort_due_to_error;
 		}
+		assert(((type1 | type2) & MEM_Real) != 0);
 		switch( pOp->opcode) {
 		case OP_Add:         rB += rA;       break;
 		case OP_Subtract:    rB -= rA;       break;
@@ -1665,9 +1663,6 @@ case OP_Remainder: {           /* same as TK_REM, in1, in2, out3 */
 		}
 		pOut->u.r = rB;
 		MemSetTypeFlag(pOut, MEM_Real);
-		if (((type1|type2)&MEM_Real)==0 && !bIntint) {
-			mem_apply_integer_type(pOut);
-		}
 	}
 	break;
 
@@ -2726,14 +2721,6 @@ case OP_Column: {
 	    default_val_mem != NULL) {
 		sqlVdbeMemShallowCopy(pDest, default_val_mem, MEM_Static);
 	}
-	if ((pDest->flags & (MEM_Int | MEM_UInt)) != 0) {
-		if (field_type == FIELD_TYPE_NUMBER) {
-			if ((pDest->flags & MEM_Int) != 0)
-				sqlVdbeMemSetDouble(pDest, pDest->u.i);
-			else
-				sqlVdbeMemSetDouble(pDest, pDest->u.u);
-		}
-	}
 	pDest->field_type = field_type;
 op_column_out:
 	REGISTER_TRACE(p, pOp->p3, pDest);
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 1393f3f..361860f 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -531,7 +531,6 @@ int sqlVdbeRealValue(Mem *, double *);
 int
 mem_value_bool(const struct Mem *mem, bool *b);
 
-int mem_apply_integer_type(Mem *);
 int sqlVdbeMemRealify(Mem *);
 int sqlVdbeMemNumerify(Mem *);
 int sqlVdbeMemCast(Mem *, enum field_type type);
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index df3f0d8..af4c1e2 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -534,23 +534,6 @@ mem_value_bool(const struct Mem *mem, bool *b)
 }
 
 /*
- * The MEM structure is already a MEM_Real.  Try to also make it a
- * MEM_Int if we can.
- */
-int
-mem_apply_integer_type(Mem *pMem)
-{
-	int rc;
-	i64 ix;
-	assert(pMem->flags & MEM_Real);
-	assert(EIGHT_BYTE_ALIGNMENT(pMem));
-
-	if ((rc = doubleToInt64(pMem->u.r, (int64_t *) &ix)) == 0)
-		mem_set_int(pMem, ix, pMem->u.r <= -1);
-	return rc;
-}
-
-/*
  * Convert pMem to type integer.  Invalidate any prior representations.
  */
 int
@@ -608,18 +591,16 @@ int
 sqlVdbeMemNumerify(Mem * pMem)
 {
 	if ((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real | MEM_Null)) == 0) {
-		assert((pMem->flags & (MEM_Blob | MEM_Str)) != 0);
+		if ((pMem->flags & (MEM_Blob | MEM_Str)) == 0)
+			return -1;
 		bool is_neg;
 		int64_t i;
 		if (sql_atoi64(pMem->z, &i, &is_neg, pMem->n) == 0) {
 			mem_set_int(pMem, i, is_neg);
 		} else {
-			double v;
-			if (sqlVdbeRealValue(pMem, &v))
+			if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n) == 0)
 				return -1;
-			pMem->u.r = v;
 			MemSetTypeFlag(pMem, MEM_Real);
-			mem_apply_integer_type(pMem);
 		}
 	}
 	assert((pMem->flags & (MEM_Int | MEM_UInt | MEM_Real | MEM_Null)) != 0);
@@ -676,22 +657,6 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 	assert(type < field_type_MAX);
 	if (pMem->flags & MEM_Null)
 		return 0;
-	if ((pMem->flags & MEM_Blob) != 0 && type == FIELD_TYPE_NUMBER) {
-		bool is_neg;
-		if (sql_atoi64(pMem->z,  (int64_t *) &pMem->u.i, &is_neg,
-			       pMem->n) == 0) {
-			MemSetTypeFlag(pMem, MEM_Real);
-			if (is_neg)
-				pMem->u.r = pMem->u.i;
-			else
-				pMem->u.r = pMem->u.u;
-			return 0;
-		}
-		if (sqlAtoF(pMem->z, &pMem->u.r, pMem->n) == 0)
-			return -1;
-		MemSetTypeFlag(pMem, MEM_Real);
-		return 0;
-	}
 	switch (type) {
 	case FIELD_TYPE_SCALAR:
 		return 0;
@@ -742,8 +707,9 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 			return -1;
 		return 0;
 	case FIELD_TYPE_DOUBLE:
-	case FIELD_TYPE_NUMBER:
 		return sqlVdbeMemRealify(pMem);
+	case FIELD_TYPE_NUMBER:
+		return sqlVdbeMemNumerify(pMem);
 	case FIELD_TYPE_VARBINARY:
 		if ((pMem->flags & MEM_Blob) != 0)
 			return 0;
diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index 23229db..43c155a 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -628,14 +628,14 @@ test:do_execsql_test(
         SELECT CAST(9223372036854774800 AS NUMBER)
     ]], {
         -- <cast-3.2>
-        9223372036854774784
+        9223372036854774800LL
         -- </cast-3.2>
     })
 
 test:do_execsql_test(
     "cast-3.3",
     "SELECT CAST(9223372036854774800 AS NUMBER)",
-    {9.22337203685477e+18})
+    {9223372036854774800LL})
 
 test:do_execsql_test(
     "cast-3.4",
@@ -643,7 +643,7 @@ test:do_execsql_test(
         SELECT CAST(CAST(9223372036854774800 AS NUMBER) AS integer)
     ]], {
         -- <cast-3.4>
-        9223372036854774784LL
+        9223372036854774800LL
         -- </cast-3.4>
     })
 
@@ -663,14 +663,14 @@ test:do_execsql_test(
         SELECT CAST(-9223372036854774800 AS NUMBER)
     ]], {
         -- <cast-3.6>
-        -9223372036854774784
+        -9223372036854774800LL
         -- </cast-3.6>
     })
 
 test:do_execsql_test(
     "cast-3.7",
     "SELECT CAST(-9223372036854774800 AS NUMBER)",
-    {-9.22337203685477e+18})
+    {-9223372036854774800LL})
 
 test:do_execsql_test(
     "cast-3.8",
@@ -678,7 +678,7 @@ test:do_execsql_test(
         SELECT CAST(CAST(-9223372036854774800 AS NUMBER) AS integer)
     ]], {
         -- <cast-3.8>
-        -9223372036854774784LL
+        -9223372036854774800LL
         -- </cast-3.8>
     })
 
@@ -695,7 +695,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "cast-3.12",
     [[
-        SELECT CAST('9223372036854774800' AS NUMBER)
+        SELECT CAST('9223372036854774800.' AS NUMBER)
     ]], {
         -- <cast-3.12>
         9223372036854774784
@@ -705,12 +705,12 @@ test:do_execsql_test(
 test:do_execsql_test(
     "cast-3.13",
     "SELECT CAST(9223372036854774800 AS NUMBER)",
-    {9.22337203685477e+18})
+    {9223372036854774800LL})
 
 test:do_execsql_test(
     "cast-3.14",
     [[
-        SELECT CAST(CAST('9223372036854774800' AS NUMBER) AS integer)
+        SELECT CAST(CAST('9223372036854774800.' AS NUMBER) AS integer)
     ]], {
         -- <cast-3.14>
         9223372036854774784LL
@@ -732,7 +732,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "cast-3.16",
     [[
-        SELECT CAST('-9223372036854774800' AS NUMBER)
+        SELECT CAST('-9223372036854774800.' AS NUMBER)
     ]], {
         -- <cast-3.16>
         -9223372036854774784
@@ -741,13 +741,13 @@ test:do_execsql_test(
 
 test:do_execsql_test(
     "cast-3.17",
-    "SELECT CAST('-9223372036854774800' AS NUMBER)",
+    "SELECT CAST('-9223372036854774800.' AS NUMBER)",
     {-9.22337203685477e+18})
 
 test:do_execsql_test(
     "cast-3.18",
     [[
-        SELECT CAST(CAST('-9223372036854774800' AS NUMBER) AS integer)
+        SELECT CAST(CAST('-9223372036854774800.' AS NUMBER) AS integer)
     ]], {
         -- <cast-3.18>
         -9223372036854774784LL
@@ -770,7 +770,7 @@ if true then --test:execsql("PRAGMA encoding")[1][1]=="UTF-8" then
     test:do_execsql_test(
         "cast-3.22",
         [[
-            SELECT CAST(x'39323233333732303336383534373734383030' AS NUMBER)
+            SELECT CAST(x'393232333337323033363835343737343830302E' AS NUMBER)
         ]], {
             -- <cast-3.22>
             9223372036854774784
@@ -778,7 +778,7 @@ if true then --test:execsql("PRAGMA encoding")[1][1]=="UTF-8" then
         })
     test:do_execsql_test(
         "cast-3.23",
-        "SELECT CAST(x'39323233333732303336383534373734383030' AS NUMBER)",
+        "SELECT CAST(x'393232333337323033363835343737343830302E' AS NUMBER)",
         {9.22337203685477e+18})
 
     test:do_execsql_test(
@@ -788,7 +788,7 @@ if true then --test:execsql("PRAGMA encoding")[1][1]=="UTF-8" then
                         AS integer)
         ]], {
             -- <cast-3.24>
-            9223372036854774784LL
+            9223372036854774800LL
             -- </cast-3.24>
         })
 end
@@ -796,7 +796,7 @@ end
 test:do_execsql_test(
     "case-3.25",
     [[
-        SELECT CAST(x'3138343436373434303733373039353531363135' AS NUMBER);
+        SELECT CAST(x'31383434363734343037333730393535313631352E' AS NUMBER);
     ]], { 1.844674407371e+19 } )
 
 test:do_execsql_test(
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index 48d8c22..c1818dd 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -760,7 +760,7 @@ test:do_execsql_test(
         INSERT INTO z1 VALUES(6, 63, '0', -26);
 
         INSERT INTO z2 VALUES(1, NULL, 21);
-        INSERT INTO z2 VALUES(2, 36, 6);
+        INSERT INTO z2 VALUES(2, 36.0, 6.0);
 
         INSERT INTO z3 VALUES(1, 123.21, 123.12);
         INSERT INTO z3 VALUES(2, 49.17, -67);
diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
index 07117d0..8ad0251 100755
--- a/test/sql-tap/numcast.test.lua
+++ b/test/sql-tap/numcast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(20)
+test:plan(31)
 
 --!./tcltestrunner.lua
 -- 2013 March 20
@@ -147,4 +147,107 @@ test:do_catchsql_test(
         1,"Tuple field 1 type does not match one required by operation: expected integer"
     })
 
+--
+-- gh-4233: Make sure that NUMBER can contain UNSIGNED, INTEGER
+-- and DOUBLE and is not automatically converted to DOUBLE.
+--
+test:do_execsql_test(
+    "numcast-2.1",
+    [[
+        CREATE TABLE t1 (id INT PRIMARY KEY, n NUMBER);
+        INSERT INTO t1 VALUES (1, 9223372036854775807);
+        INSERT INTO t1 VALUES (2, -9223372036854775807);
+        INSERT INTO t1 VALUES (3, 9223372036854775807.1);
+        SELECT n, n/100 FROM t1;
+    ]], {
+        9223372036854775807ULL, 92233720368547758ULL,
+        -9223372036854775807LL, -92233720368547758LL,
+        9223372036854775808, 92233720368547758.08
+    })
+
+test:do_execsql_test(
+    "numcast-2.2",
+    [[
+        CREATE TABLE t2(a NUMBER primary key);
+        INSERT INTO t2 VALUES(-56);
+        INSERT INTO t2 VALUES(44.0);
+        INSERT INTO t2 VALUES(46);
+        INSERT INTO t2 VALUES(56.0);
+        SELECT (a + 25) / 50 FROM t2;
+    ]], {
+        0,1.38,1,1.62
+    })
+
+test:do_execsql_test(
+    "numcast-2.3",
+    [[
+        SELECT CAST(x'3131313131313131313131313131313131313131' AS NUMBER);
+    ]], {
+        11111111111111111111ULL
+    })
+
+test:do_execsql_test(
+    "numcast-2.4",
+    [[
+        SELECT CAST(x'31313131313131313131313131313131313131312E' AS NUMBER);
+    ]], {
+        11111111111111110656
+    })
+
+test:do_execsql_test(
+    "numcast-2.5",
+    [[
+        SELECT CAST('11111111111111111111' AS NUMBER);
+    ]], {
+        11111111111111111111ULL
+    })
+
+test:do_execsql_test(
+    "numcast-2.6",
+    [[
+        SELECT CAST('101' AS NUMBER) / 10, CAST('101.' AS NUMBER) / 10;
+    ]], {
+        10, 10.1
+    })
+
+test:do_execsql_test(
+    "numcast-2.7",
+    [[
+        SELECT CAST('101     ' AS NUMBER) / 10, CAST('      101' AS NUMBER) / 10;
+    ]], {
+        10, 10
+    })
+
+test:do_execsql_test(
+    "numcast-2.8",
+    [[
+        SELECT (1 + 0) / 3, (1 + 0.) / 3, (1 + 0) / 3.;
+    ]], {
+        0, 0.33333333333333, 0.33333333333333
+    })
+
+test:do_execsql_test(
+    "numcast-2.9",
+    [[
+        SELECT (1 - 0) / 3, (1 - 0.) / 3, (1 - 0) / 3.;
+    ]], {
+        0, 0.33333333333333, 0.33333333333333
+    })
+
+test:do_execsql_test(
+    "numcast-2.10",
+    [[
+        SELECT (1 * 1) / 3, (1 * 1.) / 3, (1 * 1) / 3.;
+    ]], {
+        0, 0.33333333333333, 0.33333333333333
+    })
+
+test:do_execsql_test(
+    "numcast-2.11",
+    [[
+        SELECT (1 / 1) / 3, (1 / 1.) / 3, (1 / 1) / 3.;
+    ]], {
+        0, 0.33333333333333, 0.33333333333333
+    })
+
 test:finish_test()
diff --git a/test/sql-tap/select3.test.lua b/test/sql-tap/select3.test.lua
index 92e7f8e..10508e5 100755
--- a/test/sql-tap/select3.test.lua
+++ b/test/sql-tap/select3.test.lua
@@ -386,7 +386,7 @@ test:do_execsql_test("select3-8.1", [[
   SELECT typeof(sum(a3)) FROM a;
 ]], {
   -- <select3-8.1>
-  "number"
+  "integer"
   -- </select3-8.1>
 })
 
diff --git a/test/sql-tap/sort.test.lua b/test/sql-tap/sort.test.lua
index e156414..36074d6 100755
--- a/test/sql-tap/sort.test.lua
+++ b/test/sql-tap/sort.test.lua
@@ -243,7 +243,7 @@ test:do_execsql_test(
         SELECT v FROM t1 ORDER BY v;
     ]], {
         -- <sort-2.1.1>
-        "x-123.0", "x-2.15", "x-2b", "x-3.141592653", "x-4221.0", "x0.0013442", "x1.6", "x11.0"
+        "x-123.0", "x-2.15", "x-2b", "x-3.141592653", "x-4221.0", "x0.0013442", "x1.6", "x11"
         -- </sort-2.1.1>
     })
 
@@ -253,7 +253,7 @@ test:do_execsql_test(
         SELECT v FROM t1 ORDER BY substr(v,2,999);
     ]], {
         -- <sort-2.1.2>
-        "x-123.0", "x-2.15", "x-2b", "x-3.141592653", "x-4221.0", "x0.0013442", "x1.6", "x11.0"
+        "x-123.0", "x-2.15", "x-2b", "x-3.141592653", "x-4221.0", "x0.0013442", "x1.6", "x11"
         -- </sort-2.1.2>
     })
 
@@ -263,7 +263,7 @@ test:do_execsql_test(
         SELECT v FROM t1 ORDER BY substr(v,2,999) DESC;
     ]], {
         -- <sort-2.1.4>
-        "x11.0", "x1.6", "x0.0013442", "x-4221.0", "x-3.141592653", "x-2b", "x-2.15", "x-123.0"
+        "x11", "x1.6", "x0.0013442", "x-4221.0", "x-3.141592653", "x-2b", "x-2.15", "x-123.0"
         -- </sort-2.1.4>
     })
 
@@ -381,7 +381,7 @@ test:do_execsql_test(
         SELECT v FROM t1 ORDER BY 1;
     ]], {
         -- <sort-4.6>
-        "x-123.0", "x-2.15", "x-2b", "x-3.141592653", "x-4.0e9", "x-4221.0", "x0.0013442", "x01234567890123456789", "x1.6", "x11.0", "x2.7", "x5.0e10"
+        "x-123.0", "x-2.15", "x-2b", "x-3.141592653", "x-4.0e9", "x-4221.0", "x0.0013442", "x01234567890123456789", "x1.6", "x11", "x2.7", "x5.0e10"
         -- </sort-4.6>
     })
 
@@ -391,7 +391,7 @@ test:do_execsql_test(
         SELECT v FROM t1 ORDER BY 1 DESC;
     ]], {
         -- <sort-4.7>
-        "x5.0e10", "x2.7", "x11.0", "x1.6", "x01234567890123456789", "x0.0013442", "x-4221.0", "x-4.0e9", "x-3.141592653", "x-2b", "x-2.15", "x-123.0"
+        "x5.0e10", "x2.7", "x11", "x1.6", "x01234567890123456789", "x0.0013442", "x-4221.0", "x-4.0e9", "x-3.141592653", "x-2b", "x-2.15", "x-123.0"
         -- </sort-4.7>
     })
 
@@ -401,7 +401,7 @@ test:do_execsql_test(
         SELECT substr(v,2,99) FROM t1 ORDER BY 1;
     ]], {
         -- <sort-4.8>
-    "-123.0","-2.15","-2b","-3.141592653","-4.0e9","-4221.0","0.0013442","01234567890123456789","1.6","11.0","2.7","5.0e10"
+    "-123.0","-2.15","-2b","-3.141592653","-4.0e9","-4221.0","0.0013442","01234567890123456789","1.6","11","2.7","5.0e10"
         -- </sort-4.8>
     })
 
diff --git a/test/sql-tap/tkt-91e2e8ba6f.test.lua b/test/sql-tap/tkt-91e2e8ba6f.test.lua
index 7622f75..b12b6e0 100755
--- a/test/sql-tap/tkt-91e2e8ba6f.test.lua
+++ b/test/sql-tap/tkt-91e2e8ba6f.test.lua
@@ -35,7 +35,7 @@ test:do_execsql_test(
         SELECT x/10, y/10 FROM t1;
     ]], {
         -- <1.2>
-        1, 1.1
+        1, 1
         -- </1.2>
     })
 
@@ -45,7 +45,7 @@ test:do_execsql_test(
         SELECT x/10, y/10 FROM (SELECT * FROM t1);
     ]], {
         -- <1.3>
-        1, 1.1
+        1, 1
         -- </1.3>
     })
 
@@ -55,7 +55,7 @@ test:do_execsql_test(
         SELECT x/10, y/10 FROM (SELECT * FROM t1 LIMIT 5 OFFSET 0);
     ]], {
         -- <1.4>
-        1, 1.1
+        1, 1
         -- </1.4>
     })
 
@@ -65,7 +65,7 @@ test:do_execsql_test(
         SELECT x/10, y/10 FROM (SELECT * FROM t1 LIMIT 5 OFFSET 0) LIMIT 5 OFFSET 0;
     ]], {
         -- <1.5>
-        1, 1.1
+        1, 1
         -- </1.5>
     })
 
@@ -77,7 +77,7 @@ test:do_execsql_test(
         LIMIT 5 OFFSET 0;
     ]], {
         -- <1.6>
-        1, 1.1
+        1, 1
         -- </1.6>
     })
 
@@ -92,7 +92,7 @@ test:do_execsql_test(
         SELECT a.x/10, a.y/10 FROM v1 AS a, t1 AS b WHERE a.x = b.x LIMIT 5 OFFSET 0;
     ]], {
         -- <1.7>
-        1, 1.1
+        1, 1
         -- </1.7>
     })
 
diff --git a/test/sql/integer-overflow.result b/test/sql/integer-overflow.result
index db5c2f7..6269cb5 100644
--- a/test/sql/integer-overflow.result
+++ b/test/sql/integer-overflow.result
@@ -170,7 +170,7 @@ box.execute("SELECT * FROM t;")
   - name: A
     type: number
   rows:
-  - [1, 1.844674407371e+19]
+  - [1, 18446744073709551615]
   - [2, -1]
 ...
 box.space.T:drop()
diff --git a/test/sql/types.result b/test/sql/types.result
index 6d0aefd..89d391c 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1023,7 +1023,7 @@ box.execute("SELECT CAST(18446744073709551615 AS NUMBER);")
   - name: CAST(18446744073709551615 AS NUMBER)
     type: number
   rows:
-  - [1.844674407371e+19]
+  - [18446744073709551615]
 ...
 box.execute("SELECT CAST(18446744073709551615 AS TEXT);")
 ---
-- 
2.7.4

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik]

On 31 Dec 11:50, imeevma@tarantool.org wrote:
> This patch makes number to be union of UNSIGNED, INTEGER and
> DOUBLE numeric types.
> 
> Closes #4233
> Closes #4463

Please, explain in details changes in explicit/implicit
casts behaviour.

> ---

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Mergen Imeev]

Hi! Thank you for review. My answer below.

On Tue, Dec 31, 2019 at 10:56:07AM +0200, Nikita Pettik wrote:
> On 31 Dec 11:50, imeevma@tarantool.org wrote:
> > This patch makes number to be union of UNSIGNED, INTEGER and
> > DOUBLE numeric types.
> > 
> > Closes #4233
> > Closes #4463
> 
> Please, explain in details changes in explicit/implicit
> casts behaviour.
> 
Fixed:

commit 492646da4f781b863e5e3e488a0be53f31d28964
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Oct 26 17:27:53 2019 +0300

    sql: make NUMBER to be union of SQL numeric types
    
    This patch makes number to be union of UNSIGNED, INTEGER and
    DOUBLE numeric types.
    
    Closes #4233
    Closes #4463
    
    @TarantoolBot document
    Title: NUMBER type in SQL.
    
    The NUMBER type in SQL is the union of all currently available
    numeric types in SQL: INTEGER, UNSIGNED, and DOUBLE. The types
    INTEGER and DOUBLE can be called subtypes of type NUMBER. Any
    value of type NUMBER has a subtype of INTEGER or DOUBLE. When
    any numeric value is implicitly or explicitly casted to NUMBER,
    the only thing that can change is its type. Its type will become
    NUMBER. When a value of type NUMBER is cast explicitly or
    implicitly to other numeric types, the rules applicable to the
    cast are determined by the subtype of the value.
    
    If a value of type STRING can be implicitly cast to value of
    type INTEGER or DOUBLE, then this value can be cast explicitly
    and implicitly to a value of type NUMBER. If this value can be
    implicitly cast to INTEGER, then its subtype will be INTEGER.
    The subtype will be DOUBLE in another case.
    
    If a value of type VARBINARY can be explicitly cast to type
    INTEGER or DOUBLE, then this value can be explicitly cast to a
    value of type NUMBER. If this value can be explicitly cast to
    INTEGER, then its subtype will be INTEGER. The subtype will be
    DOUBLE in another case.
> > ---

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik]

On 31 Dec 12:46, Mergen Imeev wrote:
> Hi! Thank you for review. My answer below.
> 
> On Tue, Dec 31, 2019 at 10:56:07AM +0200, Nikita Pettik wrote:
> > On 31 Dec 11:50, imeevma@tarantool.org wrote:
> > > This patch makes number to be union of UNSIGNED, INTEGER and
> > > DOUBLE numeric types.
> > > 
> > > Closes #4233
> > > Closes #4463
> > 
> > Please, explain in details changes in explicit/implicit
> > casts behaviour.
> > 
> Fixed:
> 
> commit 492646da4f781b863e5e3e488a0be53f31d28964
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Sat Oct 26 17:27:53 2019 +0300
> 
>     sql: make NUMBER to be union of SQL numeric types
>     
>     This patch makes number to be union of UNSIGNED, INTEGER and
>     DOUBLE numeric types.
>     
>     Closes #4233
>     Closes #4463
>     
>     @TarantoolBot document
>     Title: NUMBER type in SQL.

Could you please intead of 3 paragraphs just include following
changelog list:

"This patch makes definition of NUMBER type be consistent with NoSQL.
 Previously (for historic reasons) in half-cases NUMBER served as a
 synonym of DOUBLE type.
 After patch is applied following will change:
 - CAST AS NUMBER operation no more longer results in any value
   change in case value is already of numeric type. Example:
   Obsolete behaviour:
	SELECT CAST(922337206854774800 AS NUMBER);
        Result: 9223372036854774784
   New behaviour:
        SELECT CAST(922337206854774800 AS NUMBER);                              
        Result: 9223372036854774800LL
 - ...
"

Or group implicit/explicit cast changes. Text below is quite
compicated to perceive (IMHO).

>     
>     The NUMBER type in SQL is the union of all currently available
>     numeric types in SQL: INTEGER, UNSIGNED, and DOUBLE. The types
>     INTEGER and DOUBLE can be called subtypes of type NUMBER. Any
>     value of type NUMBER has a subtype of INTEGER or DOUBLE. When
>     any numeric value is implicitly or explicitly casted to NUMBER,
>     the only thing that can change is its type. Its type will become
>     NUMBER. When a value of type NUMBER is cast explicitly or
>     implicitly to other numeric types, the rules applicable to the
>     cast are determined by the subtype of the value.
>     
>     If a value of type STRING can be implicitly cast to value of
>     type INTEGER or DOUBLE, then this value can be cast explicitly
>     and implicitly to a value of type NUMBER. If this value can be
>     implicitly cast to INTEGER, then its subtype will be INTEGER.
>     The subtype will be DOUBLE in another case.
>     
>     If a value of type VARBINARY can be explicitly cast to type
>     INTEGER or DOUBLE, then this value can be explicitly cast to a
>     value of type NUMBER. If this value can be explicitly cast to
>     INTEGER, then its subtype will be INTEGER. The subtype will be
>     DOUBLE in another case.
> > > ---

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Mergen Imeev]

Hi! Thank you for review. My answer and new commit-message
below.

On Mon, Jan 13, 2020 at 03:58:29PM +0300, Nikita Pettik wrote:
> On 31 Dec 12:46, Mergen Imeev wrote:
> > Hi! Thank you for review. My answer below.
> > 
> > On Tue, Dec 31, 2019 at 10:56:07AM +0200, Nikita Pettik wrote:
> > > On 31 Dec 11:50, imeevma@tarantool.org wrote:
> > > > This patch makes number to be union of UNSIGNED, INTEGER and
> > > > DOUBLE numeric types.
> > > > 
> > > > Closes #4233
> > > > Closes #4463
> > > 
> > > Please, explain in details changes in explicit/implicit
> > > casts behaviour.
> > > 
> > Fixed:
> > 
> > commit 492646da4f781b863e5e3e488a0be53f31d28964
> > Author: Mergen Imeev <imeevma@gmail.com>
> > Date:   Sat Oct 26 17:27:53 2019 +0300
> > 
> >     sql: make NUMBER to be union of SQL numeric types
> >     
> >     This patch makes number to be union of UNSIGNED, INTEGER and
> >     DOUBLE numeric types.
> >     
> >     Closes #4233
> >     Closes #4463
> >     
> >     @TarantoolBot document
> >     Title: NUMBER type in SQL.
> 
> Could you please intead of 3 paragraphs just include following
> changelog list:
> 
> "This patch makes definition of NUMBER type be consistent with NoSQL.
>  Previously (for historic reasons) in half-cases NUMBER served as a
>  synonym of DOUBLE type.
>  After patch is applied following will change:
>  - CAST AS NUMBER operation no more longer results in any value
>    change in case value is already of numeric type. Example:
>    Obsolete behaviour:
> 	SELECT CAST(922337206854774800 AS NUMBER);
>         Result: 9223372036854774784
>    New behaviour:
>         SELECT CAST(922337206854774800 AS NUMBER);                              
>         Result: 9223372036854774800LL
>  - ...
> "
> 
> Or group implicit/explicit cast changes. Text below is quite
> compicated to perceive (IMHO).
> 
I removed the part that describes the cast from STRING and
VARBINARY to NUMBER, since I'm not sure if this is the
correct behavior. I think it will be fixed in #3809 and
#4230 issues:
https://github.com/tarantool/tarantool/issues/3809
https://github.com/tarantool/tarantool/issues/4230

> >     
> >     The NUMBER type in SQL is the union of all currently available
> >     numeric types in SQL: INTEGER, UNSIGNED, and DOUBLE. The types
> >     INTEGER and DOUBLE can be called subtypes of type NUMBER. Any
> >     value of type NUMBER has a subtype of INTEGER or DOUBLE. When
> >     any numeric value is implicitly or explicitly casted to NUMBER,
> >     the only thing that can change is its type. Its type will become
> >     NUMBER. When a value of type NUMBER is cast explicitly or
> >     implicitly to other numeric types, the rules applicable to the
> >     cast are determined by the subtype of the value.
> >     
> >     If a value of type STRING can be implicitly cast to value of
> >     type INTEGER or DOUBLE, then this value can be cast explicitly
> >     and implicitly to a value of type NUMBER. If this value can be
> >     implicitly cast to INTEGER, then its subtype will be INTEGER.
> >     The subtype will be DOUBLE in another case.
> >     
> >     If a value of type VARBINARY can be explicitly cast to type
> >     INTEGER or DOUBLE, then this value can be explicitly cast to a
> >     value of type NUMBER. If this value can be explicitly cast to
> >     INTEGER, then its subtype will be INTEGER. The subtype will be
> >     DOUBLE in another case.
> > > > ---

New commit-message:

commit ac5e4fcffe17767efa7da77427024e2d679bebf7
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Oct 26 17:27:53 2019 +0300

    sql: make NUMBER to be union of SQL numeric types
    
    This patch makes definition of NUMBER type be consistent with
    NoSQL. Previously (for historic reasons) in half-cases NUMBER
    served as a synonym of DOUBLE type.
    
    After patch is applied CAST AS NUMBER operation no more longer
    results in any value change in case value is already of numeric
    type.
    
    Example:
    Obsolete behaviour:
    	SELECT CAST(922337206854774800 AS NUMBER);
    	Result: 9223372036854774784
    New behaviour:
    	SELECT CAST(922337206854774800 AS NUMBER);
    	Result: 9223372036854774800LL
    
    Closes #4233
    Closes #4463

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik]

On 17 Jan 11:06, Mergen Imeev wrote:
> Hi! Thank you for review. My answer and new commit-message
> below.
> 
> > > commit 492646da4f781b863e5e3e488a0be53f31d28964
> > > Author: Mergen Imeev <imeevma@gmail.com>
> > > Date:   Sat Oct 26 17:27:53 2019 +0300
> > > 
> > >     sql: make NUMBER to be union of SQL numeric types
> > >     
> > >     This patch makes number to be union of UNSIGNED, INTEGER and
> > >     DOUBLE numeric types.
> > >     
> > >     Closes #4233
> > >     Closes #4463
> > >     
> > >     @TarantoolBot document
> > >     Title: NUMBER type in SQL.
> > 
> > Could you please intead of 3 paragraphs just include following
> > changelog list:
> > 
> > "This patch makes definition of NUMBER type be consistent with NoSQL.
> >  Previously (for historic reasons) in half-cases NUMBER served as a
> >  synonym of DOUBLE type.
> >  After patch is applied following will change:
> >  - CAST AS NUMBER operation no more longer results in any value
> >    change in case value is already of numeric type. Example:
> >    Obsolete behaviour:
> > 	SELECT CAST(922337206854774800 AS NUMBER);
> >         Result: 9223372036854774784
> >    New behaviour:
> >         SELECT CAST(922337206854774800 AS NUMBER);                              
> >         Result: 9223372036854774800LL
> >  - ...
> > "
> > 
> > Or group implicit/explicit cast changes. Text below is quite
> > compicated to perceive (IMHO).
> > 
> I removed the part that describes the cast from STRING and
> VARBINARY to NUMBER, since I'm not sure if this is the
> correct behavior. I think it will be fixed in #3809 and
> #4230 issues:
> https://github.com/tarantool/tarantool/issues/3809
> https://github.com/tarantool/tarantool/issues/4230

Does this statement imply any code changes?
 
> > >     The NUMBER type in SQL is the union of all currently available
> > >     numeric types in SQL: INTEGER, UNSIGNED, and DOUBLE. The types
> > >     INTEGER and DOUBLE can be called subtypes of type NUMBER. Any
> > >     value of type NUMBER has a subtype of INTEGER or DOUBLE. When
> > >     any numeric value is implicitly or explicitly casted to NUMBER,
> > >     the only thing that can change is its type. Its type will become
> > >     NUMBER. When a value of type NUMBER is cast explicitly or
> > >     implicitly to other numeric types, the rules applicable to the
> > >     cast are determined by the subtype of the value.
> > >     
> > >     If a value of type STRING can be implicitly cast to value of
> > >     type INTEGER or DOUBLE, then this value can be cast explicitly
> > >     and implicitly to a value of type NUMBER. If this value can be
> > >     implicitly cast to INTEGER, then its subtype will be INTEGER.
> > >     The subtype will be DOUBLE in another case.
> > >     
> > >     If a value of type VARBINARY can be explicitly cast to type
> > >     INTEGER or DOUBLE, then this value can be explicitly cast to a
> > >     value of type NUMBER. If this value can be explicitly cast to
> > >     INTEGER, then its subtype will be INTEGER. The subtype will be
> > >     DOUBLE in another case.
> > > > > ---
> 
> New commit-message:
> 
> commit ac5e4fcffe17767efa7da77427024e2d679bebf7
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Sat Oct 26 17:27:53 2019 +0300
> 
>     sql: make NUMBER to be union of SQL numeric types
>     
>     This patch makes definition of NUMBER type be consistent with
>     NoSQL. Previously (for historic reasons) in half-cases NUMBER
>     served as a synonym of DOUBLE type.
>     
>     After patch is applied CAST AS NUMBER operation no more longer
>     results in any value change in case value is already of numeric
>     type.
>     
>     Example:
>     Obsolete behaviour:
>     	SELECT CAST(922337206854774800 AS NUMBER);
>     	Result: 9223372036854774784
>     New behaviour:
>     	SELECT CAST(922337206854774800 AS NUMBER);
>     	Result: 9223372036854774800LL

Is CAST the only operation is affected in scope of your patch?
According to test changes - no. Please, provide an example on each
user-visible change (you might not notice ellipsis in my remark).

>     
>     Closes #4233
>     Closes #4463

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Mergen Imeev]

Hi! Thank you for review. My answers and new commit-message
below.

On Wed, Jan 22, 2020 at 04:26:17PM +0300, Nikita Pettik wrote:
> On 17 Jan 11:06, Mergen Imeev wrote:
> > Hi! Thank you for review. My answer and new commit-message
> > below.
> > 
> > > > commit 492646da4f781b863e5e3e488a0be53f31d28964
> > > > Author: Mergen Imeev <imeevma@gmail.com>
> > > > Date:   Sat Oct 26 17:27:53 2019 +0300
> > > > 
> > > >     sql: make NUMBER to be union of SQL numeric types
> > > >     
> > > >     This patch makes number to be union of UNSIGNED, INTEGER and
> > > >     DOUBLE numeric types.
> > > >     
> > > >     Closes #4233
> > > >     Closes #4463
> > > >     
> > > >     @TarantoolBot document
> > > >     Title: NUMBER type in SQL.
> > > 
> > > Could you please intead of 3 paragraphs just include following
> > > changelog list:
> > > 
> > > "This patch makes definition of NUMBER type be consistent with NoSQL.
> > >  Previously (for historic reasons) in half-cases NUMBER served as a
> > >  synonym of DOUBLE type.
> > >  After patch is applied following will change:
> > >  - CAST AS NUMBER operation no more longer results in any value
> > >    change in case value is already of numeric type. Example:
> > >    Obsolete behaviour:
> > > 	SELECT CAST(922337206854774800 AS NUMBER);
> > >         Result: 9223372036854774784
> > >    New behaviour:
> > >         SELECT CAST(922337206854774800 AS NUMBER);                              
> > >         Result: 9223372036854774800LL
> > >  - ...
> > > "
> > > 
> > > Or group implicit/explicit cast changes. Text below is quite
> > > compicated to perceive (IMHO).
> > > 
> > I removed the part that describes the cast from STRING and
> > VARBINARY to NUMBER, since I'm not sure if this is the
> > correct behavior. I think it will be fixed in #3809 and
> > #4230 issues:
> > https://github.com/tarantool/tarantool/issues/3809
> > https://github.com/tarantool/tarantool/issues/4230
> 
> Does this statement imply any code changes?
>  
Can't say for now. It is possible, since now it is
disallowed to explicitly cast from binary to DOUBLE, and
allowed to cast from BINARY to INTEGER.

> > > >     The NUMBER type in SQL is the union of all currently available
> > > >     numeric types in SQL: INTEGER, UNSIGNED, and DOUBLE. The types
> > > >     INTEGER and DOUBLE can be called subtypes of type NUMBER. Any
> > > >     value of type NUMBER has a subtype of INTEGER or DOUBLE. When
> > > >     any numeric value is implicitly or explicitly casted to NUMBER,
> > > >     the only thing that can change is its type. Its type will become
> > > >     NUMBER. When a value of type NUMBER is cast explicitly or
> > > >     implicitly to other numeric types, the rules applicable to the
> > > >     cast are determined by the subtype of the value.
> > > >     
> > > >     If a value of type STRING can be implicitly cast to value of
> > > >     type INTEGER or DOUBLE, then this value can be cast explicitly
> > > >     and implicitly to a value of type NUMBER. If this value can be
> > > >     implicitly cast to INTEGER, then its subtype will be INTEGER.
> > > >     The subtype will be DOUBLE in another case.
> > > >     
> > > >     If a value of type VARBINARY can be explicitly cast to type
> > > >     INTEGER or DOUBLE, then this value can be explicitly cast to a
> > > >     value of type NUMBER. If this value can be explicitly cast to
> > > >     INTEGER, then its subtype will be INTEGER. The subtype will be
> > > >     DOUBLE in another case.
> > > > > > ---
> > 
> > New commit-message:
> > 
> > commit ac5e4fcffe17767efa7da77427024e2d679bebf7
> > Author: Mergen Imeev <imeevma@gmail.com>
> > Date:   Sat Oct 26 17:27:53 2019 +0300
> > 
> >     sql: make NUMBER to be union of SQL numeric types
> >     
> >     This patch makes definition of NUMBER type be consistent with
> >     NoSQL. Previously (for historic reasons) in half-cases NUMBER
> >     served as a synonym of DOUBLE type.
> >     
> >     After patch is applied CAST AS NUMBER operation no more longer
> >     results in any value change in case value is already of numeric
> >     type.
> >     
> >     Example:
> >     Obsolete behaviour:
> >     	SELECT CAST(922337206854774800 AS NUMBER);
> >     	Result: 9223372036854774784
> >     New behaviour:
> >     	SELECT CAST(922337206854774800 AS NUMBER);
> >     	Result: 9223372036854774800LL
> 
> Is CAST the only operation is affected in scope of your patch?
> According to test changes - no. Please, provide an example on each
> user-visible change (you might not notice ellipsis in my remark).
> 
Fixed.

> >     
> >     Closes #4233
> >     Closes #4463

New commit-message:

commit 9982ca9c6e239089e425a16bc5c7b5f9ab847fc4
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Oct 26 17:27:53 2019 +0300

    sql: make NUMBER to be union of SQL numeric types
    
    This patch makes definition of NUMBER type be consistent with
    NoSQL. Previously (for historic reasons) in half-cases NUMBER
    served as a synonym of DOUBLE type.
    
    After patch is applied CAST AS NUMBER operation no more longer
    results in any value change in case value is already of numeric
    type.
    
    Examples:
    1) CAST function:
    
    Obsolete behaviour:
    tarantool> SELECT CAST(922337206854774800 AS NUMBER), CAST(5 AS NUMBER) / 10;
    ---
    - metadata:
      - name: CAST(922337206854774800 AS NUMBER)
        type: number
      - name: CAST(5 AS NUMBER) / 10
        type: number
      rows:
      - [922337206854774784, 0.5]
    ...
    
    New behaviour:
    tarantool> SELECT CAST(922337206854774800 AS NUMBER), CAST(5 AS NUMBER) / 10;
    ---
    - metadata:
      - name: CAST(922337206854774800 AS NUMBER)
        type: number
      - name: CAST(5 AS NUMBER) / 10
        type: number
      rows:
      - [922337206854774800, 0]
    ...
    
    2) Table column:
    
    Obsolete behaviour:
    tarantool> CREATE TABLE t (n NUMBER PRIMARY KEY);
    ---
    - row_count: 1
    ...
    
    tarantool> INSERT INTO t VALUES (3), (-4), (5.0);
    ---
    - row_count: 3
    ...
    
    tarantool> SELECT n, n/10 FROM t;
    ---
    - metadata:
      - name: N
        type: number
      - name: n/10
        type: number
      rows:
      - [-4, -0.4]
      - [3, 0.3]
      - [5, 0.5]
    ...
    
    New behaviour:
    tarantool> CREATE TABLE t (n NUMBER PRIMARY KEY);
    ---
    - row_count: 1
    ...
    
    tarantool> INSERT INTO t VALUES (3), (-4), (5.0);
    ---
    - row_count: 3
    ...
    
    tarantool> SELECT n, n/10 FROM t;
    ---
    - metadata:
      - name: N
        type: number
      - name: n/10
        type: number
      rows:
      - [-4, 0]
      - [3, 0]
      - [5, 0.5]
    ...
    
    Closes #4233
    Closes #4463

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik]

On 23 Jan 16:40, Mergen Imeev wrote:
> Hi! Thank you for review. My answers and new commit-message
> below.

Hi,

I've a bit reworked your patch: splitted it into consistent sequence of
independent patches and provided refactoring for sqlVdbeMemNumerify().
Also I found that CAST boolean AS NUMBER didn't work properly: cast resulted
in error meanshile cast boolean as integer is legal, so that as number should
be legal as well. There's no more functional changes except for mentioned one.
Please look at branch: https://github.com/tarantool/tarantool/commits/np/gh-4233-fix-number-field-type-in-sql

If it is OK to you please reply and I will push patches to master bracnch.
Thanks. 
 

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Mergen Imeev]

On Tue, Jan 28, 2020 at 07:23:29PM +0300, Nikita Pettik wrote:
> On 23 Jan 16:40, Mergen Imeev wrote:
> > Hi! Thank you for review. My answers and new commit-message
> > below.
> 
> Hi,
> 
> I've a bit reworked your patch: splitted it into consistent sequence of
> independent patches and provided refactoring for sqlVdbeMemNumerify().
> Also I found that CAST boolean AS NUMBER didn't work properly: cast resulted
> in error meanshile cast boolean as integer is legal, so that as number should
> be legal as well. There's no more functional changes except for mentioned one.
> Please look at branch: https://github.com/tarantool/tarantool/commits/np/gh-4233-fix-number-field-type-in-sql
> 
> If it is OK to you please reply and I will push patches to master bracnch.
> Thanks. 
>  
Hi,

I'm sorry, but I do not like it. Since now it is yours
commits, I'll ask Sasha to move issues #4233 and #4463
to you. If you want a review, please ask Vlad.

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik]

On 03 Feb 12:52, Mergen Imeev wrote:
> On Tue, Jan 28, 2020 at 07:23:29PM +0300, Nikita Pettik wrote:
> > On 23 Jan 16:40, Mergen Imeev wrote:
> > > Hi! Thank you for review. My answers and new commit-message
> > > below.
> > 
> > Hi,
> > 
> > I've a bit reworked your patch: splitted it into consistent sequence of
> > independent patches and provided refactoring for sqlVdbeMemNumerify().
> > Also I found that CAST boolean AS NUMBER didn't work properly: cast resulted
> > in error meanshile cast boolean as integer is legal, so that as number should
> > be legal as well. There's no more functional changes except for mentioned one.
> > Please look at branch: https://github.com/tarantool/tarantool/commits/np/gh-4233-fix-number-field-type-in-sql
> > 
> > If it is OK to you please reply and I will push patches to master bracnch.
> > Thanks. 
> >  
> Hi,
> 
> I'm sorry, but I do not like it.

Fair review.

> Since now it is yours
> commits, I'll ask Sasha to move issues #4233 and #4463
> to you. If you want a review, please ask Vlad.

Okay, let's spend one more iteration cycle.

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Alexander Turenko]

On Mon, Feb 03, 2020 at 12:52:10PM +0300, Mergen Imeev wrote:
> On Tue, Jan 28, 2020 at 07:23:29PM +0300, Nikita Pettik wrote:
> > On 23 Jan 16:40, Mergen Imeev wrote:
> > > Hi! Thank you for review. My answers and new commit-message
> > > below.
> > 
> > Hi,
> > 
> > I've a bit reworked your patch: splitted it into consistent sequence of
> > independent patches and provided refactoring for sqlVdbeMemNumerify().
> > Also I found that CAST boolean AS NUMBER didn't work properly: cast resulted
> > in error meanshile cast boolean as integer is legal, so that as number should
> > be legal as well. There's no more functional changes except for mentioned one.
> > Please look at branch: https://github.com/tarantool/tarantool/commits/np/gh-4233-fix-number-field-type-in-sql
> > 
> > If it is OK to you please reply and I will push patches to master bracnch.
> > Thanks. 
> >  
> Hi,
> 
> I'm sorry, but I do not like it. Since now it is yours
> commits, I'll ask Sasha to move issues #4233 and #4463
> to you. If you want a review, please ask Vlad.

I guess Nikita expects that you'll agree with splitting of the commits
and show you the code to save a time on a discussion. Since you
disagreed (this fast path fails), I think Nikita should provide
objections against your way explicitly. (However 'I do not like' is very
weak argument, it is always better to provide some reasons explicitly.)

'CAST boolean AS NUMBER' does not look as part of tasks Mergen solving
here (#4233 and #4463). Aside of this, the commit that fixes it is named
as 'sql: refactor sqlVdbeMemNumerify()', but it changes a user visible
behaviour and so it is not pure refactoring. It is also strange that it
has no tests that will show how the behaviour is changed.

Changes related to Tarantool/SQL type system are very opaque now in the
sense that we have no formal type system description we want to
implement as result. Just 'fix that' and 'change this'. I think we
should design the type system first (we should did it a year ago) and
only then make some changes. CCed Kirill and Sergos to plan this
activity (or decide that the current way to design it is okay).

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH v2 1/1] sql: make NUMBER to be union of SQL numeric types

[Nikita Pettik]

On 10 Feb 00:29, Alexander Turenko wrote:
> On Mon, Feb 03, 2020 at 12:52:10PM +0300, Mergen Imeev wrote:
> > On Tue, Jan 28, 2020 at 07:23:29PM +0300, Nikita Pettik wrote:
> > > On 23 Jan 16:40, Mergen Imeev wrote:
> > > > Hi! Thank you for review. My answers and new commit-message
> > > > below.
> > > 
> > > Hi,
> > > 
> > > I've a bit reworked your patch: splitted it into consistent sequence of
> > > independent patches and provided refactoring for sqlVdbeMemNumerify().
> > > Also I found that CAST boolean AS NUMBER didn't work properly: cast resulted
> > > in error meanshile cast boolean as integer is legal, so that as number should
> > > be legal as well. There's no more functional changes except for mentioned one.
> > > Please look at branch: https://github.com/tarantool/tarantool/commits/np/gh-4233-fix-number-field-type-in-sql
> > > 
> > > If it is OK to you please reply and I will push patches to master bracnch.
> > > Thanks. 
> > >  
> > Hi,
> > 
> > I'm sorry, but I do not like it. Since now it is yours
> > commits, I'll ask Sasha to move issues #4233 and #4463
> > to you. If you want a review, please ask Vlad.
> 
> I guess Nikita expects that you'll agree with splitting of the commits
> and show you the code to save a time on a discussion. Since you
> disagreed (this fast path fails), I think Nikita should provide

I've provided arguments in my review (as I always do).

> objections against your way explicitly. (However 'I do not like' is very
> weak argument, it is always better to provide some reasons explicitly.)
> 
> 'CAST boolean AS NUMBER' does not look as part of tasks Mergen solving
> here (#4233 and #4463).

Changes provided by Mergen look significant enough to consider them as
reworking of number type. So as long as cast blob to number is changed in
scope of patch, it seems to be ok to fix cast boolean to number as well.

>Aside of this, the commit that fixes it is named
> as 'sql: refactor sqlVdbeMemNumerify()', but it changes a user visible
> behaviour and so it is not pure refactoring.

Could you please explain why it "changes a user visible behaviour"? :)
What is more, commit message explicitly says:

"...allow conversion from boolean to number
    (since it is legal to convert boolean to integer, and in turn number
    type completely includes integer type)."

It is related only to this function, which in turn is unreachable.
If it is not clear enough, I'll fix a bit commit message.
Finally, if you wish to stress the word 'refactoring' implying its
'non-functional' origins..Well, as I already said, patch does not
introduce any functional changes (i.e. changes that can be tested), but in
current context 'refactoring' may seem a bit dubious. I guess replacing
it with 'rework' is enough to remove any doubts.

>It is also strange that it
> has no tests that will show how the behaviour is changed.

It's a consequence from your previous *wrong* observation: patch has no tests
since it doesn't change anything that can be tested. Tests which verify
boolean to number cast operation come with patch that introduces this
change (not the one we are speaking about).
 
> Changes related to Tarantool/SQL type system are very opaque now in the
> sense that we have no formal type system description we want to
> implement as result. Just 'fix that' and 'change this'. I think we
> should design the type system first (we should did it a year ago) and
> only then make some changes.

Well we (me, Kostja and Peter) had a long conversation last summer concerning
types in SQL (it is still available in mailing list). It is resulted in
following issues:
https://github.com/tarantool/tarantool/issues/3809
https://github.com/tarantool/tarantool/issues/4230

Alternatively, implicit casts can be completely removed (which matches
with ANSI on the one hand, but contradicts to user's exprience on
the other hand). Both options look acceptable to me.

Speaking of "type system", in fact there's well stablished plan. What we
need to do to finish it is (in terms of major issues):

 - remove implicit casts (or reconsider them; now they are a bit broken -
   see issues #3809 and #4230);
 - decide how to threat SCALAR type in SQL (on the discussion,
   see Re: [Tarantool-discussions] Implicit cast for COMPARISON);
 - implement MAP and ARRAY types (#4762 and #4763);
 - implement DECIMAL type (#4415).

Then we will be able to move to some more advanced features like varchars,
datetime, user-defined-cast operations and so forth (depending on customer's
requests).

Mergen's patch in particular resolves the legacy issues appeared due
to the absence of DOUBLE type in Tarantool. Owing to that "number"
type in SQL used to emulate DOUBLE type; since now double is on board,
we can revert that behaviour.

> CCed Kirill and Sergos to plan this
> activity (or decide that the current way to design it is okay).
> 
> WBR, Alexander Turenko.