Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER
@ 2020-03-27 11:33 imeevma
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

This patch-set fixes CAST() from BLOB to INTEGER in case a BLOB
does not have '\0'.

https://github.com/tarantool/tarantool/issues/4766
https://github.com/tarantool/tarantool/tree/imeevma/gh-4766-fix-blob-size-for-cast

@Changelog
 - Explicit and implicit cast from string contains DOUBLE value to
   INTEGER or UNSIGNED was disallowed.
 - Maximum length of a BLOB that is allowed to be cast to INTEGER
   or UNSIGNED was limited to 12287 bytes.
 - Fixed wrong behaviour of CAST() from BLOB to INTEGER in case a
   BLOB does not have '\0'. (gh-4766)

Mergen Imeev (3):
  sql: fix CAST() from STRING to INTEGER
  sql: fix implicit cast from STRING to INTEGER
  sql: add '\0' to the BLOB when it is cast to INTEGER

 src/box/sql/util.c                                 |  17 ++-
 src/box/sql/vdbe.c                                 |  11 +-
 src/box/sql/vdbeInt.h                              |   1 -
 src/box/sql/vdbemem.c                              |  45 ++-----
 test/sql-tap/e_select1.test.lua                    |   2 +-
 .../gh-4766-wrong-cast-from-blob-to-int.test.lua   | 150 +++++++++++++++++++++
 test/sql-tap/intpkey.test.lua                      |   2 +-
 test/sql-tap/join.test.lua                         |   4 +-
 test/sql-tap/subquery.test.lua                     |   6 +-
 test/sql-tap/tkt-9a8b09f8e6.test.lua               |   4 +-
 test/sql/types.result                              |  23 ++--
 11 files changed, 203 insertions(+), 62 deletions(-)
 create mode 100755 test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua

-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma
@ 2020-03-27 11:33 ` imeevma
  2020-03-27 16:46   ` Nikita Pettik
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

Prior to this patch, STRING, which contains the DOUBLE value,
could be cast to INTEGER. This was done by converting STRING to
DOUBLE and then converting this DOUBLE value to INTEGER. This may
affect the accuracy of CAST(), so it was forbidden.

Before patch:
box.execute("SELECT CAST('111.1' as INTEGER);")
Result: 111

After patch:
box.execute("SELECT CAST('1.1' as INTEGER);")
Result: 'Type mismatch: can not convert 1.1 to integer'

box.execute("SELECT CAST('1.0' as INTEGER);")
Result: 'Type mismatch: can not convert 1.0 to integer'

box.execute("SELECT CAST('1.' as INTEGER);")
Result: 'Type mismatch: can not convert 1. to integer'

Part of #4766
---
 src/box/sql/vdbemem.c                              | 16 +++++-
 .../gh-4766-wrong-cast-from-blob-to-int.test.lua   | 57 ++++++++++++++++++++++
 test/sql/types.result                              | 23 ++++-----
 3 files changed, 80 insertions(+), 16 deletions(-)
 create mode 100755 test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index aad030d..de1d9c3 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 		return -1;
 	case FIELD_TYPE_INTEGER:
 	case FIELD_TYPE_UNSIGNED:
-		if ((pMem->flags & MEM_Blob) != 0) {
+		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
 			bool is_neg;
 			int64_t val;
 			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
@@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 			MemSetTypeFlag(pMem, MEM_UInt);
 			return 0;
 		}
-		if (sqlVdbeMemIntegerify(pMem) != 0)
+		if ((pMem->flags & MEM_Real) != 0) {
+			double d;
+			if (sqlVdbeRealValue(pMem, &d) != 0)
+				return -1;
+			if (d < INT64_MAX && d >= INT64_MIN) {
+				mem_set_int(pMem, d, d <= -1);
+				return 0;
+			}
+			if (d >= INT64_MAX && d < UINT64_MAX) {
+				mem_set_u64(pMem, d);
+				return 0;
+			}
 			return -1;
+		}
 		if (type == FIELD_TYPE_UNSIGNED &&
 		    (pMem->flags & MEM_UInt) == 0)
 			return -1;
diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
new file mode 100755
index 0000000..0865c4e
--- /dev/null
+++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
@@ -0,0 +1,57 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(6)
+
+--
+-- Make sure that STRING or BLOB that contains DOUBLE value cannot
+-- be cast to INTEGER.
+--
+test:do_catchsql_test(
+    "gh-4766-1",
+    [[
+        SELECT CAST('1.1' AS INTEGER)
+    ]], {
+        1, "Type mismatch: can not convert 1.1 to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-2",
+    [[
+        SELECT CAST(x'312e31' AS INTEGER)
+    ]], {
+        1, "Type mismatch: can not convert varbinary to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-3",
+    [[
+        SELECT CAST('2.0' AS INTEGER)
+    ]], {
+        1, "Type mismatch: can not convert 2.0 to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-4",
+    [[
+        SELECT CAST(x'322e30' AS INTEGER)
+    ]], {
+        1, "Type mismatch: can not convert varbinary to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-5",
+    [[
+        SELECT CAST('2.' AS INTEGER)
+    ]], {
+        1, "Type mismatch: can not convert 2. to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-6",
+    [[
+        SELECT CAST(x'322e' AS INTEGER)
+    ]], {
+        1, "Type mismatch: can not convert varbinary to integer"
+    })
+
+test:finish_test()
diff --git a/test/sql/types.result b/test/sql/types.result
index 38e4385..54aff46 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -269,11 +269,8 @@ box.space.T1:drop()
 --
 box.execute("SELECT CAST('1.123' AS INTEGER);")
 ---
-- metadata:
-  - name: CAST('1.123' AS INTEGER)
-    type: integer
-  rows:
-  - [1]
+- null
+- 'Type mismatch: can not convert 1.123 to integer'
 ...
 box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
 ---
@@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
 ...
 box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
 ---
-- metadata:
-  - name: CAST(f AS INTEGER)
-    type: integer
-  rows:
-  - [0]
-  - [1]
-  - [3]
+- null
+- 'Type mismatch: can not convert 0.0 to integer'
 ...
 box.space.T1:drop()
 ---
@@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
 ...
 box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
 ---
-- null
-- 'Type mismatch: can not convert -1 to unsigned'
+- metadata:
+  - name: CAST(-1.5 AS UNSIGNED)
+    type: unsigned
+  rows:
+  - [-1]
 ...
 box.execute("SELECT CAST(true AS UNSIGNED);")
 ---
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER
  2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma
@ 2020-03-27 11:33 ` imeevma
  2020-03-27 16:54   ` Nikita Pettik
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma
  2020-04-16  0:03 ` [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " Nikita Pettik
  3 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

Prior to this patch, STRING, which contains the DOUBLE value,
could be implicitly cast to INTEGER. This was done by converting
STRING to DOUBLE and then converting this DOUBLE value to INTEGER.
This may affect the accuracy of CAST(), so it was forbidden.

Example:
box.execute("CREATE TABLE t(i INT PRIMARY KEY);")

Before patch:
box.execute("INSERT INTO t VALUES ('111.1');")
box.execute("SELECT * FROM t;")
Result: 111

After patch:
box.execute("INSERT INTO t VALUES ('1.1');")
Result: 'Type mismatch: can not convert 1.1 to integer'

box.execute("INSERT INTO t VALUES ('1.0');")
Result: 'Type mismatch: can not convert 1.0 to integer'

box.execute("INSERT INTO t VALUES ('1.');")
Result: 'Type mismatch: can not convert 1. to integer'

Part of #4766

@TarantoolBot document
Title: disallow cast from STRING contais DOUBLE to INTEGER

After the last two patches, explicit and implicit casting from the
string containing DOUBLE to INTEGER directly will be prohibited.
The user must use the explicit cast to DOUBLE before the explicit
or implicit cast to INTEGER. The reason for this is that before
these patches, such STRINGs were implicitly cast to DOUBLE, and
then this DOUBLE was implicitly or explicitly cast to INTEGER.
Because of this, the result of such a cast may differ from what
the user expects, and the user may not know why.

Example for implicit cast:

box.execute("CREATE TABLE t(i INT PRIMARY KEY);")
-- Does not work anymore:
box.execute("INSERT INTO t VALUES ('1.1');")
-- Right way:
box.execute("INSERT INTO t VALUES (CAST('1.1' AS DOUBLE));")

Example for explicit cast:

-- Does not work anymore:
box.execute("SELECT CAST('1.1' AS INTEGER);")
-- Right way:
box.execute("SELECT CAST(CAST('1.1' AS DOUBLE) AS INTEGER);")
---
 src/box/sql/vdbe.c                                 | 11 ++++-
 src/box/sql/vdbeInt.h                              |  1 -
 src/box/sql/vdbemem.c                              | 29 ------------
 test/sql-tap/e_select1.test.lua                    |  2 +-
 .../gh-4766-wrong-cast-from-blob-to-int.test.lua   | 55 +++++++++++++++++++++-
 test/sql-tap/intpkey.test.lua                      |  2 +-
 test/sql-tap/join.test.lua                         |  4 +-
 test/sql-tap/subquery.test.lua                     |  6 +--
 test/sql-tap/tkt-9a8b09f8e6.test.lua               |  4 +-
 9 files changed, 72 insertions(+), 42 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a..6c0e5bd 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -319,6 +319,8 @@ mem_apply_type(struct Mem *record, enum field_type type)
 	switch (type) {
 	case FIELD_TYPE_INTEGER:
 	case FIELD_TYPE_UNSIGNED:
+		if ((record->flags & (MEM_Bool | MEM_Blob)) != 0)
+			return -1;
 		if ((record->flags & MEM_UInt) == MEM_UInt)
 			return 0;
 		if ((record->flags & MEM_Real) == MEM_Real) {
@@ -331,8 +333,13 @@ mem_apply_type(struct Mem *record, enum field_type type)
 				mem_set_u64(record, u);
 			return 0;
 		}
-		if (sqlVdbeMemIntegerify(record) != 0)
-			return -1;
+		if ((record->flags & MEM_Str) != 0) {
+			bool is_neg;
+			int64_t i;
+			if (sql_atoi64(record->z, &i, &is_neg, record->n) != 0)
+				return -1;
+			mem_set_int(record, i, is_neg);
+		}
 		if ((record->flags & MEM_Int) == MEM_Int) {
 			if (type == FIELD_TYPE_UNSIGNED)
 				return -1;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 38305ce..2c50b67 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -525,7 +525,6 @@ int sqlVdbeMemMakeWriteable(Mem *);
 int sqlVdbeMemStringify(Mem *);
 int sqlVdbeIntValue(Mem *, int64_t *, bool *is_neg);
 
-int sqlVdbeMemIntegerify(struct Mem *pMem);
 int sqlVdbeRealValue(Mem *, double *);
 
 int
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index de1d9c3..3bc7303 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -551,35 +551,6 @@ mem_apply_integer_type(Mem *pMem)
 }
 
 /*
- * Convert pMem to type integer.  Invalidate any prior representations.
- */
-int
-sqlVdbeMemIntegerify(struct Mem *pMem)
-{
-	assert(EIGHT_BYTE_ALIGNMENT(pMem));
-
-	int64_t i;
-	bool is_neg;
-	if (sqlVdbeIntValue(pMem, &i, &is_neg) == 0) {
-		mem_set_int(pMem, i, is_neg);
-		return 0;
-	}
-
-	double d;
-	if (sqlVdbeRealValue(pMem, &d) != 0)
-		return -1;
-	if (d < INT64_MAX && d >= INT64_MIN) {
-		mem_set_int(pMem, d, d <= -1);
-		return 0;
-	}
-	if (d >= INT64_MAX && d < UINT64_MAX) {
-		mem_set_u64(pMem, d);
-		return 0;
-	}
-	return -1;
-}
-
-/*
  * Convert pMem so that it is of type MEM_Real.
  * Invalidate any prior representations.
  */
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index c1818dd..1d3b964 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -2195,7 +2195,7 @@ test:do_select_tests(
         {"1", "SELECT b FROM f1 ORDER BY a LIMIT 0 ", {}},
         {"2", "SELECT b FROM f1 ORDER BY a DESC LIMIT 4 ", {"z", "y", "x", "w"}},
         {"3", "SELECT b FROM f1 ORDER BY a DESC LIMIT 8 ", {"z", "y", "x", "w", "v", "u", "t", "s"}},
-        {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12.0' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}},
+        {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}},
     })
 
 -- EVIDENCE-OF: R-54935-19057 Or, if the SELECT statement would return
diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
index 0865c4e..559e33d 100755
--- a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
+++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(6)
+test:plan(12)
 
 --
 -- Make sure that STRING or BLOB that contains DOUBLE value cannot
@@ -54,4 +54,57 @@ test:do_catchsql_test(
         1, "Type mismatch: can not convert varbinary to integer"
     })
 
+--
+-- Make sure that STRING or BLOB that contains DOUBLE value cannot
+-- be implicitly cast to INTEGER.
+--
+test:do_catchsql_test(
+    "gh-4766-7",
+    [[
+        CREATE TABLE t(i INT PRIMARY KEY);
+        INSERT INTO t VALUES ('1.1');
+    ]], {
+        1, "Type mismatch: can not convert 1.1 to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-8",
+    [[
+        INSERT INTO t VALUES (x'312e31');
+    ]], {
+        1, "Type mismatch: can not convert varbinary to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-9",
+    [[
+        INSERT INTO t VALUES ('2.0');
+    ]], {
+        1, "Type mismatch: can not convert 2.0 to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-10",
+    [[
+        INSERT INTO t VALUES (x'322e30');
+    ]], {
+        1, "Type mismatch: can not convert varbinary to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-11",
+    [[
+        INSERT INTO t VALUES ('2.');
+    ]], {
+        1, "Type mismatch: can not convert 2. to integer"
+    })
+
+test:do_catchsql_test(
+    "gh-4766-12",
+    [[
+        INSERT INTO t VALUES (x'322e');
+    ]], {
+        1, "Type mismatch: can not convert varbinary to integer"
+    })
+
 test:finish_test()
diff --git a/test/sql-tap/intpkey.test.lua b/test/sql-tap/intpkey.test.lua
index bec2670..b6b1866 100755
--- a/test/sql-tap/intpkey.test.lua
+++ b/test/sql-tap/intpkey.test.lua
@@ -788,7 +788,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "intpkey-13.2",
     [[
-        INSERT INTO t1 VALUES('1.0',2,3);
+        INSERT INTO t1 VALUES('1',2,3);
         SELECT * FROM t1 WHERE a=1;
     ]], {
         -- <intpkey-13.2>
diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua
index 4f014e0..840b780 100755
--- a/test/sql-tap/join.test.lua
+++ b/test/sql-tap/join.test.lua
@@ -1017,7 +1017,7 @@ test:do_test(
         return test:execsql [[
             CREATE TABLE t1(a TEXT primary key, b TEXT);
             CREATE TABLE t2(b INTEGER primary key, a TEXT);
-            INSERT INTO t1 VALUES('one', '1.0');
+            INSERT INTO t1 VALUES('one', '1');
             INSERT INTO t1 VALUES('two', '2');
             INSERT INTO t2 VALUES(1, 'one');
             INSERT INTO t2 VALUES(2, 'two');
@@ -1034,7 +1034,7 @@ test:do_execsql_test(
         SELECT * FROM t1 NATURAL JOIN t2 
     ]], {
         -- <join-11.9>
-        "one", "1.0", "two", "2"
+        "one", "1", "two", "2"
         -- </join-11.9>
     })
 
diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
index 6bedf58..15c4c82 100755
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-tap/subquery.test.lua
@@ -342,7 +342,7 @@ test:do_execsql_test(
         INSERT INTO t3 VALUES(10);
 
         CREATE TABLE t4(x TEXT PRIMARY KEY);
-        INSERT INTO t4 VALUES('10.0');
+        INSERT INTO t4 VALUES('10');
     ]], {
         -- <subquery-2.5.1>
         
@@ -363,7 +363,7 @@ test:do_test(
         ]]
     end, {
         -- <subquery-2.5.2>
-        "10.0"
+        "10"
         -- </subquery-2.5.2>
     })
 
@@ -378,7 +378,7 @@ test:do_test(
         ]]
     end, {
         -- <subquery-2.5.3.1>
-        "10.0"
+        "10"
         -- </subquery-2.5.3.1>
     })
 
diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
index db0881c..cb5348a 100755
--- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
+++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
@@ -196,7 +196,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     3.4,
     [[
-        SELECT x FROM t2 WHERE x IN ('1.0');
+        SELECT x FROM t2 WHERE x IN ('1');
     ]], {
         -- <3.4>
         1
@@ -236,7 +236,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     3.8,
     [[
-        SELECT x FROM t2 WHERE '1.0' IN (x);
+        SELECT x FROM t2 WHERE '1' IN (x);
     ]], {
         -- <3.8>
         1
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast to INTEGER
  2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma
@ 2020-03-27 11:33 ` imeevma
  2020-03-27 16:54   ` Nikita Pettik
  2020-04-16  0:03 ` [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " Nikita Pettik
  3 siblings, 1 reply; 18+ messages in thread
From: imeevma @ 2020-03-27 11:33 UTC (permalink / raw)
  To: korablev, tsafin, tarantool-patches

Prior to this patch, due to the absence of the '\0' character at
the end of the BLOB, it was possible to get an error or incorrect
result when using CAST() from BLOB to INTEGER or UNSIGNED. This
has now been fixed, but the maximum length of a BLOB that could be
cast to INTEGER or UNSIGNED was limited to 12287 bytes.

Examples of wrong CAST() from BLOB to INTEGER:

CREATE TABLE t (i INT PRIMARY KEY, a VARBINARY, b INT, c INT);
INSERT INTO t VALUES (1, X'33', 0x33, 0x00), (2, X'34', 0x41, 0);

Example of wrong result:

SELECT CAST(a AS INTEGER) FROM t WHERE i = 1;

Result: 33

Example of error during CAST():

SELECT CAST(a AS INTEGER) FROM t WHERE i = 2;

Result: 'Type mismatch: can not convert varbinary to integer'

Closes #4766
---
 src/box/sql/util.c                                 | 17 ++++++---
 .../gh-4766-wrong-cast-from-blob-to-int.test.lua   | 42 +++++++++++++++++++++-
 2 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/util.c b/src/box/sql/util.c
index f908e9c..c556b98 100644
--- a/src/box/sql/util.c
+++ b/src/box/sql/util.c
@@ -467,14 +467,21 @@ sql_atoi64(const char *z, int64_t *val, bool *is_neg, int length)
 	if (*z == '-')
 		*is_neg = true;
 
+	/*
+	 * BLOB data may not end with '\0'. Because of this, the
+	 * strtoll() and strtoull() functions may return an
+	 * incorrect result. To fix this, let's copy the value for
+	 * decoding into static memory and add '\0' to it.
+	 */
+	if (length > SMALL_STATIC_SIZE - 1)
+		return -1;
+	const char *str_value = tt_cstr(z, length);
 	char *end = NULL;
 	errno = 0;
-	if (*z == '-') {
-		*is_neg = true;
-		*val = strtoll(z, &end, 10);
+	if (*is_neg) {
+		*val = strtoll(str_value, &end, 10);
 	} else {
-		*is_neg = false;
-		uint64_t u_val = strtoull(z, &end, 10);
+		uint64_t u_val = strtoull(str_value, &end, 10);
 		*val = u_val;
 	}
 	/* Overflow and underflow errors. */
diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
index 559e33d..37aae87 100755
--- a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
+++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(12)
+test:plan(15)
 
 --
 -- Make sure that STRING or BLOB that contains DOUBLE value cannot
@@ -107,4 +107,44 @@ test:do_catchsql_test(
         1, "Type mismatch: can not convert varbinary to integer"
     })
 
+--
+-- Make sure that a blob as part of a tuple can be cast to NUMBER,
+-- INTEGER and UNSIGNED. Prior to this patch, an error could
+-- appear due to the absence of '\0' at the end of the BLOB.
+--
+test:do_execsql_test(
+    "gh-4766-13",
+    [[
+        CREATE TABLE t1 (a VARBINARY PRIMARY KEY);
+        INSERT INTO t1 VALUES (X'33'), (X'372020202020');
+        SELECT a, CAST(a AS NUMBER), CAST(a AS INTEGER), CAST(a AS UNSIGNED) FROM t1;
+    ]], {
+        '3', 3, 3, 3, '7     ', 7, 7, 7
+    })
+
+--
+-- Make sure that BLOB longer than 12287 bytes cannot be cast to
+-- INTEGER.
+--
+long_str = string.rep('0', 12284)
+test:do_execsql_test(
+    "gh-4766-14",
+    "SELECT CAST('" .. long_str .. "123'" .. " AS INTEGER);", {
+        123
+    })
+
+
+test:do_catchsql_test(
+    "gh-4766-15",
+    "SELECT CAST('" .. long_str .. "1234'" .. " AS INTEGER);", {
+        1, "Type mismatch: can not convert 000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "0000000000000000000000000000000000000000000000000000000000000000000" ..
+        "000000000000000000000000000000000000000000000"
+    })
+
 test:finish_test()
-- 
2.7.4

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma
@ 2020-03-27 16:46   ` Nikita Pettik
  2020-04-10 10:39     ` Mergen Imeev
  0 siblings, 1 reply; 18+ messages in thread
From: Nikita Pettik @ 2020-03-27 16:46 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

On 27 Mar 14:33, imeevma@tarantool.org wrote:

Could you please find Peter's table containing current/expected cast
behaviours and verify that this patch doesn't contradict it?

> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index aad030d..de1d9c3 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
>  		return -1;
>  	case FIELD_TYPE_INTEGER:
>  	case FIELD_TYPE_UNSIGNED:
> -		if ((pMem->flags & MEM_Blob) != 0) {
> +		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
>  			bool is_neg;
>  			int64_t val;
>  			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
> @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
>  			MemSetTypeFlag(pMem, MEM_UInt);
>  			return 0;
>  		}
> -		if (sqlVdbeMemIntegerify(pMem) != 0)
> +		if ((pMem->flags & MEM_Real) != 0) {
> +			double d;
> +			if (sqlVdbeRealValue(pMem, &d) != 0)
> +				return -1;
> +			if (d < INT64_MAX && d >= INT64_MIN) {
> +				mem_set_int(pMem, d, d <= -1);
> +				return 0;
> +			}
> +			if (d >= INT64_MAX && d < UINT64_MAX) {
> +				mem_set_u64(pMem, d);
> +				return 0;
> +			}
>  			return -1;

Instead of keeping inlining code into sqlVdbeMemCast() I'd better
split it into separate functions. Good refactoring task tho - just
keep in mind.

> +		}
>  		if (type == FIELD_TYPE_UNSIGNED &&
>  		    (pMem->flags & MEM_UInt) == 0)
>  			return -1;
> diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> new file mode 100755
> index 0000000..0865c4e
> --- /dev/null
> +++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua

Strictly speaking this change is not related to 4766, so I'd not
create separate test file for current patch. Moreover, tests in
sql/types.test.lua seem to cover it.

> +#!/usr/bin/env tarantool
> +test = require("sqltester")
> +test:plan(6)
> +
> +--
> +-- Make sure that STRING or BLOB that contains DOUBLE value cannot
> +-- be cast to INTEGER.
> +--
> +test:do_catchsql_test(
> +    "gh-4766-1",
> +    [[
> +        SELECT CAST('1.1' AS INTEGER)
> +    ]], {
> +        1, "Type mismatch: can not convert 1.1 to integer"
> +    })
> +
> +test:do_catchsql_test(
> +    "gh-4766-2",
> +    [[
> +        SELECT CAST(x'312e31' AS INTEGER)
> +    ]], {
> +        1, "Type mismatch: can not convert varbinary to integer"
> +    })
> +
> diff --git a/test/sql/types.result b/test/sql/types.result
> index 38e4385..54aff46 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -269,11 +269,8 @@ box.space.T1:drop()
>  --
>  box.execute("SELECT CAST('1.123' AS INTEGER);")
>  ---
> -- metadata:
> -  - name: CAST('1.123' AS INTEGER)
> -    type: integer
> -  rows:
> -  - [1]
> +- null
> +- 'Type mismatch: can not convert 1.123 to integer'
>  ...
>  box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
>  ---
> @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
>  ...
>  box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
>  ---
> -- metadata:
> -  - name: CAST(f AS INTEGER)
> -    type: integer
> -  rows:
> -  - [0]
> -  - [1]
> -  - [3]
> +- null
> +- 'Type mismatch: can not convert 0.0 to integer'
>  ...
>  box.space.T1:drop()
>  ---
> @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
>  ...
>  box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
>  ---
> -- null
> -- 'Type mismatch: can not convert -1 to unsigned'
> +- metadata:
> +  - name: CAST(-1.5 AS UNSIGNED)
> +    type: unsigned
> +  rows:
> +  - [-1]
>  ...
>  box.execute("SELECT CAST(true AS UNSIGNED);")
>  ---
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma
@ 2020-03-27 16:54   ` Nikita Pettik
  2020-04-10 10:41     ` Mergen Imeev
  0 siblings, 1 reply; 18+ messages in thread
From: Nikita Pettik @ 2020-03-27 16:54 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

On 27 Mar 14:33, imeevma@tarantool.org wrote:
> Prior to this patch, STRING, which contains the DOUBLE value,
> could be implicitly cast to INTEGER. This was done by converting
> STRING to DOUBLE and then converting this DOUBLE value to INTEGER.
> This may affect the accuracy of CAST(), so it was forbidden.
> 
> Example:
> box.execute("CREATE TABLE t(i INT PRIMARY KEY);")
> 
> Before patch:
> box.execute("INSERT INTO t VALUES ('111.1');")
> box.execute("SELECT * FROM t;")
> Result: 111
> 
> After patch:
> box.execute("INSERT INTO t VALUES ('1.1');")
> Result: 'Type mismatch: can not convert 1.1 to integer'
> 
> box.execute("INSERT INTO t VALUES ('1.0');")
> Result: 'Type mismatch: can not convert 1.0 to integer'
> 
> box.execute("INSERT INTO t VALUES ('1.');")
> Result: 'Type mismatch: can not convert 1. to integer'

Is comparison predicat affected?

> Part of #4766
> 
> @TarantoolBot document
> Title: disallow cast from STRING contais DOUBLE to INTEGER

-> contains
 
> diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> index 0865c4e..559e33d 100755
> --- a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> +++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua

Same nit concerning test file as in previous patch: personally I'd
not introduce it.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast to INTEGER
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma
@ 2020-03-27 16:54   ` Nikita Pettik
  0 siblings, 0 replies; 18+ messages in thread
From: Nikita Pettik @ 2020-03-27 16:54 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

On 27 Mar 14:33, imeevma@tarantool.org wrote:

LGTM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-03-27 16:46   ` Nikita Pettik
@ 2020-04-10 10:39     ` Mergen Imeev
  2020-04-10 10:43       ` Mergen Imeev
  2020-04-10 12:46       ` Nikita Pettik
  0 siblings, 2 replies; 18+ messages in thread
From: Mergen Imeev @ 2020-04-10 10:39 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches

Hi! Thank you for review. My answers and new patch below.

On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
> On 27 Mar 14:33, imeevma@tarantool.org wrote:
> 
> Could you please find Peter's table containing current/expected cast
> behaviours and verify that this patch doesn't contradict it?
> 
> > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > index aad030d..de1d9c3 100644
> > --- a/src/box/sql/vdbemem.c
> > +++ b/src/box/sql/vdbemem.c
> > @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> >  		return -1;
> >  	case FIELD_TYPE_INTEGER:
> >  	case FIELD_TYPE_UNSIGNED:
> > -		if ((pMem->flags & MEM_Blob) != 0) {
> > +		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
> >  			bool is_neg;
> >  			int64_t val;
> >  			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
> > @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> >  			MemSetTypeFlag(pMem, MEM_UInt);
> >  			return 0;
> >  		}
> > -		if (sqlVdbeMemIntegerify(pMem) != 0)
> > +		if ((pMem->flags & MEM_Real) != 0) {
> > +			double d;
> > +			if (sqlVdbeRealValue(pMem, &d) != 0)
> > +				return -1;
> > +			if (d < INT64_MAX && d >= INT64_MIN) {
> > +				mem_set_int(pMem, d, d <= -1);
> > +				return 0;
> > +			}
> > +			if (d >= INT64_MAX && d < UINT64_MAX) {
> > +				mem_set_u64(pMem, d);
> > +				return 0;
> > +			}
> >  			return -1;
> 
> Instead of keeping inlining code into sqlVdbeMemCast() I'd better
> split it into separate functions. Good refactoring task tho - just
> keep in mind.
> 
Thanks. I will fix this in #3809 issue.

> > +		}
> >  		if (type == FIELD_TYPE_UNSIGNED &&
> >  		    (pMem->flags & MEM_UInt) == 0)
> >  			return -1;
> > diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> > new file mode 100755
> > index 0000000..0865c4e
> > --- /dev/null
> > +++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> 
> Strictly speaking this change is not related to 4766, so I'd not
> create separate test file for current patch. Moreover, tests in
> sql/types.test.lua seem to cover it.
> 
I removed this file.

> > +#!/usr/bin/env tarantool
> > +test = require("sqltester")
> > +test:plan(6)
> > +
> > +--
> > +-- Make sure that STRING or BLOB that contains DOUBLE value cannot
> > +-- be cast to INTEGER.
> > +--
> > +test:do_catchsql_test(
> > +    "gh-4766-1",
> > +    [[
> > +        SELECT CAST('1.1' AS INTEGER)
> > +    ]], {
> > +        1, "Type mismatch: can not convert 1.1 to integer"
> > +    })
> > +
> > +test:do_catchsql_test(
> > +    "gh-4766-2",
> > +    [[
> > +        SELECT CAST(x'312e31' AS INTEGER)
> > +    ]], {
> > +        1, "Type mismatch: can not convert varbinary to integer"
> > +    })
> > +
> > diff --git a/test/sql/types.result b/test/sql/types.result
> > index 38e4385..54aff46 100644
> > --- a/test/sql/types.result
> > +++ b/test/sql/types.result
> > @@ -269,11 +269,8 @@ box.space.T1:drop()
> >  --
> >  box.execute("SELECT CAST('1.123' AS INTEGER);")
> >  ---
> > -- metadata:
> > -  - name: CAST('1.123' AS INTEGER)
> > -    type: integer
> > -  rows:
> > -  - [1]
> > +- null
> > +- 'Type mismatch: can not convert 1.123 to integer'
> >  ...
> >  box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
> >  ---
> > @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
> >  ...
> >  box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
> >  ---
> > -- metadata:
> > -  - name: CAST(f AS INTEGER)
> > -    type: integer
> > -  rows:
> > -  - [0]
> > -  - [1]
> > -  - [3]
> > +- null
> > +- 'Type mismatch: can not convert 0.0 to integer'
> >  ...
> >  box.space.T1:drop()
> >  ---
> > @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
> >  ...
> >  box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
> >  ---
> > -- null
> > -- 'Type mismatch: can not convert -1 to unsigned'
> > +- metadata:
> > +  - name: CAST(-1.5 AS UNSIGNED)
> > +    type: unsigned
> > +  rows:
> > +  - [-1]
> >  ...
> >  box.execute("SELECT CAST(true AS UNSIGNED);")
> >  ---
> > -- 
> > 2.7.4
> > 


New patch:


From d2679859651aeee2dffda01545e6c62ae3c185d1 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 16 Mar 2020 15:12:37 +0300
Subject: [PATCH] sql: fix CAST() from STRING to INTEGER

Prior to this patch, STRING, which contains the DOUBLE value,
could be cast to INTEGER. This was done by converting STRING to
DOUBLE and then converting this DOUBLE value to INTEGER. This may
affect the accuracy of CAST(), so it was forbidden.

Before patch:
box.execute("SELECT CAST('111.1' as INTEGER);")
Result: 111

After patch:
box.execute("SELECT CAST('1.1' as INTEGER);")
Result: 'Type mismatch: can not convert 1.1 to integer'

box.execute("SELECT CAST('1.0' as INTEGER);")
Result: 'Type mismatch: can not convert 1.0 to integer'

box.execute("SELECT CAST('1.' as INTEGER);")
Result: 'Type mismatch: can not convert 1. to integer'

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index aad030d..de1d9c3 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 		return -1;
 	case FIELD_TYPE_INTEGER:
 	case FIELD_TYPE_UNSIGNED:
-		if ((pMem->flags & MEM_Blob) != 0) {
+		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
 			bool is_neg;
 			int64_t val;
 			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
@@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
 			MemSetTypeFlag(pMem, MEM_UInt);
 			return 0;
 		}
-		if (sqlVdbeMemIntegerify(pMem) != 0)
+		if ((pMem->flags & MEM_Real) != 0) {
+			double d;
+			if (sqlVdbeRealValue(pMem, &d) != 0)
+				return -1;
+			if (d < INT64_MAX && d >= INT64_MIN) {
+				mem_set_int(pMem, d, d <= -1);
+				return 0;
+			}
+			if (d >= INT64_MAX && d < UINT64_MAX) {
+				mem_set_u64(pMem, d);
+				return 0;
+			}
 			return -1;
+		}
 		if (type == FIELD_TYPE_UNSIGNED &&
 		    (pMem->flags & MEM_UInt) == 0)
 			return -1;
diff --git a/test/sql/types.result b/test/sql/types.result
index 38e4385..54aff46 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -269,11 +269,8 @@ box.space.T1:drop()
 --
 box.execute("SELECT CAST('1.123' AS INTEGER);")
 ---
-- metadata:
-  - name: CAST('1.123' AS INTEGER)
-    type: integer
-  rows:
-  - [1]
+- null
+- 'Type mismatch: can not convert 1.123 to integer'
 ...
 box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
 ---
@@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
 ...
 box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
 ---
-- metadata:
-  - name: CAST(f AS INTEGER)
-    type: integer
-  rows:
-  - [0]
-  - [1]
-  - [3]
+- null
+- 'Type mismatch: can not convert 0.0 to integer'
 ...
 box.space.T1:drop()
 ---
@@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
 ...
 box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
 ---
-- null
-- 'Type mismatch: can not convert -1 to unsigned'
+- metadata:
+  - name: CAST(-1.5 AS UNSIGNED)
+    type: unsigned
+  rows:
+  - [-1]
 ...
 box.execute("SELECT CAST(true AS UNSIGNED);")
 ---

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER
  2020-03-27 16:54   ` Nikita Pettik
@ 2020-04-10 10:41     ` Mergen Imeev
  2020-04-10 12:57       ` Nikita Pettik
  0 siblings, 1 reply; 18+ messages in thread
From: Mergen Imeev @ 2020-04-10 10:41 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches

On Fri, Mar 27, 2020 at 04:54:17PM +0000, Nikita Pettik wrote:
> On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > Prior to this patch, STRING, which contains the DOUBLE value,
> > could be implicitly cast to INTEGER. This was done by converting
> > STRING to DOUBLE and then converting this DOUBLE value to INTEGER.
> > This may affect the accuracy of CAST(), so it was forbidden.
> > 
> > Example:
> > box.execute("CREATE TABLE t(i INT PRIMARY KEY);")
> > 
> > Before patch:
> > box.execute("INSERT INTO t VALUES ('111.1');")
> > box.execute("SELECT * FROM t;")
> > Result: 111
> > 
> > After patch:
> > box.execute("INSERT INTO t VALUES ('1.1');")
> > Result: 'Type mismatch: can not convert 1.1 to integer'
> > 
> > box.execute("INSERT INTO t VALUES ('1.0');")
> > Result: 'Type mismatch: can not convert 1.0 to integer'
> > 
> > box.execute("INSERT INTO t VALUES ('1.');")
> > Result: 'Type mismatch: can not convert 1. to integer'
> 
> Is comparison predicat affected?
> 
No, since all implicit casts in case of comparison executed
inside of comparisons opcodes.

> > Part of #4766
> > 
> > @TarantoolBot document
> > Title: disallow cast from STRING contais DOUBLE to INTEGER
> 
> -> contains
>  
Fixed.

> > diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> > index 0865c4e..559e33d 100755
> > --- a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> > +++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> 
> Same nit concerning test file as in previous patch: personally I'd
> not introduce it.
> 
Removed.


New patch:


From 18aed8fc1b1c566a90888c2835e4210ef3386009 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Thu, 26 Mar 2020 15:30:02 +0300
Subject: [PATCH] sql: fix implicit cast from STRING to INTEGER

Prior to this patch, STRING, which contains the DOUBLE value,
could be implicitly cast to INTEGER. This was done by converting
STRING to DOUBLE and then converting this DOUBLE value to INTEGER.
This may affect the accuracy of CAST(), so it was forbidden.

Example:
box.execute("CREATE TABLE t(i INT PRIMARY KEY);")

Before patch:
box.execute("INSERT INTO t VALUES ('111.1');")
box.execute("SELECT * FROM t;")
Result: 111

After patch:
box.execute("INSERT INTO t VALUES ('1.1');")
Result: 'Type mismatch: can not convert 1.1 to integer'

box.execute("INSERT INTO t VALUES ('1.0');")
Result: 'Type mismatch: can not convert 1.0 to integer'

box.execute("INSERT INTO t VALUES ('1.');")
Result: 'Type mismatch: can not convert 1. to integer'

@TarantoolBot document
Title: disallow cast from STRING contains DOUBLE to INTEGER

After the last two patches, explicit and implicit casting from the
string containing DOUBLE to INTEGER directly will be prohibited.
The user must use the explicit cast to DOUBLE before the explicit
or implicit cast to INTEGER. The reason for this is that before
these patches, such STRINGs were implicitly cast to DOUBLE, and
then this DOUBLE was implicitly or explicitly cast to INTEGER.
Because of this, the result of such a cast may differ from what
the user expects, and the user may not know why.

Example for implicit cast:

box.execute("CREATE TABLE t(i INT PRIMARY KEY);")
-- Does not work anymore:
box.execute("INSERT INTO t VALUES ('1.1');")
-- Right way:
box.execute("INSERT INTO t VALUES (CAST('1.1' AS DOUBLE));")

Example for explicit cast:

-- Does not work anymore:
box.execute("SELECT CAST('1.1' AS INTEGER);")
-- Right way:
box.execute("SELECT CAST(CAST('1.1' AS DOUBLE) AS INTEGER);")

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index e8a029a..6c0e5bd 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -319,6 +319,8 @@ mem_apply_type(struct Mem *record, enum field_type type)
 	switch (type) {
 	case FIELD_TYPE_INTEGER:
 	case FIELD_TYPE_UNSIGNED:
+		if ((record->flags & (MEM_Bool | MEM_Blob)) != 0)
+			return -1;
 		if ((record->flags & MEM_UInt) == MEM_UInt)
 			return 0;
 		if ((record->flags & MEM_Real) == MEM_Real) {
@@ -331,8 +333,13 @@ mem_apply_type(struct Mem *record, enum field_type type)
 				mem_set_u64(record, u);
 			return 0;
 		}
-		if (sqlVdbeMemIntegerify(record) != 0)
-			return -1;
+		if ((record->flags & MEM_Str) != 0) {
+			bool is_neg;
+			int64_t i;
+			if (sql_atoi64(record->z, &i, &is_neg, record->n) != 0)
+				return -1;
+			mem_set_int(record, i, is_neg);
+		}
 		if ((record->flags & MEM_Int) == MEM_Int) {
 			if (type == FIELD_TYPE_UNSIGNED)
 				return -1;
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 38305ce..2c50b67 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -525,7 +525,6 @@ int sqlVdbeMemMakeWriteable(Mem *);
 int sqlVdbeMemStringify(Mem *);
 int sqlVdbeIntValue(Mem *, int64_t *, bool *is_neg);
 
-int sqlVdbeMemIntegerify(struct Mem *pMem);
 int sqlVdbeRealValue(Mem *, double *);
 
 int
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index de1d9c3..3bc7303 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -551,35 +551,6 @@ mem_apply_integer_type(Mem *pMem)
 }
 
 /*
- * Convert pMem to type integer.  Invalidate any prior representations.
- */
-int
-sqlVdbeMemIntegerify(struct Mem *pMem)
-{
-	assert(EIGHT_BYTE_ALIGNMENT(pMem));
-
-	int64_t i;
-	bool is_neg;
-	if (sqlVdbeIntValue(pMem, &i, &is_neg) == 0) {
-		mem_set_int(pMem, i, is_neg);
-		return 0;
-	}
-
-	double d;
-	if (sqlVdbeRealValue(pMem, &d) != 0)
-		return -1;
-	if (d < INT64_MAX && d >= INT64_MIN) {
-		mem_set_int(pMem, d, d <= -1);
-		return 0;
-	}
-	if (d >= INT64_MAX && d < UINT64_MAX) {
-		mem_set_u64(pMem, d);
-		return 0;
-	}
-	return -1;
-}
-
-/*
  * Convert pMem so that it is of type MEM_Real.
  * Invalidate any prior representations.
  */
diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
index c1818dd..1d3b964 100755
--- a/test/sql-tap/e_select1.test.lua
+++ b/test/sql-tap/e_select1.test.lua
@@ -2195,7 +2195,7 @@ test:do_select_tests(
         {"1", "SELECT b FROM f1 ORDER BY a LIMIT 0 ", {}},
         {"2", "SELECT b FROM f1 ORDER BY a DESC LIMIT 4 ", {"z", "y", "x", "w"}},
         {"3", "SELECT b FROM f1 ORDER BY a DESC LIMIT 8 ", {"z", "y", "x", "w", "v", "u", "t", "s"}},
-        {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12.0' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}},
+        {"4", "SELECT b FROM f1 ORDER BY a DESC LIMIT '12' ", {"z", y, "x", "w", "v", "u", "t", "s", "r", "q", "p", "o"}},
     })
 
 -- EVIDENCE-OF: R-54935-19057 Or, if the SELECT statement would return
diff --git a/test/sql-tap/intpkey.test.lua b/test/sql-tap/intpkey.test.lua
index bec2670..b6b1866 100755
--- a/test/sql-tap/intpkey.test.lua
+++ b/test/sql-tap/intpkey.test.lua
@@ -788,7 +788,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     "intpkey-13.2",
     [[
-        INSERT INTO t1 VALUES('1.0',2,3);
+        INSERT INTO t1 VALUES('1',2,3);
         SELECT * FROM t1 WHERE a=1;
     ]], {
         -- <intpkey-13.2>
diff --git a/test/sql-tap/join.test.lua b/test/sql-tap/join.test.lua
index 4f014e0..840b780 100755
--- a/test/sql-tap/join.test.lua
+++ b/test/sql-tap/join.test.lua
@@ -1017,7 +1017,7 @@ test:do_test(
         return test:execsql [[
             CREATE TABLE t1(a TEXT primary key, b TEXT);
             CREATE TABLE t2(b INTEGER primary key, a TEXT);
-            INSERT INTO t1 VALUES('one', '1.0');
+            INSERT INTO t1 VALUES('one', '1');
             INSERT INTO t1 VALUES('two', '2');
             INSERT INTO t2 VALUES(1, 'one');
             INSERT INTO t2 VALUES(2, 'two');
@@ -1034,7 +1034,7 @@ test:do_execsql_test(
         SELECT * FROM t1 NATURAL JOIN t2 
     ]], {
         -- <join-11.9>
-        "one", "1.0", "two", "2"
+        "one", "1", "two", "2"
         -- </join-11.9>
     })
 
diff --git a/test/sql-tap/subquery.test.lua b/test/sql-tap/subquery.test.lua
index 6bedf58..15c4c82 100755
--- a/test/sql-tap/subquery.test.lua
+++ b/test/sql-tap/subquery.test.lua
@@ -342,7 +342,7 @@ test:do_execsql_test(
         INSERT INTO t3 VALUES(10);
 
         CREATE TABLE t4(x TEXT PRIMARY KEY);
-        INSERT INTO t4 VALUES('10.0');
+        INSERT INTO t4 VALUES('10');
     ]], {
         -- <subquery-2.5.1>
         
@@ -363,7 +363,7 @@ test:do_test(
         ]]
     end, {
         -- <subquery-2.5.2>
-        "10.0"
+        "10"
         -- </subquery-2.5.2>
     })
 
@@ -378,7 +378,7 @@ test:do_test(
         ]]
     end, {
         -- <subquery-2.5.3.1>
-        "10.0"
+        "10"
         -- </subquery-2.5.3.1>
     })
 
diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
index db0881c..cb5348a 100755
--- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
+++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
@@ -196,7 +196,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     3.4,
     [[
-        SELECT x FROM t2 WHERE x IN ('1.0');
+        SELECT x FROM t2 WHERE x IN ('1');
     ]], {
         -- <3.4>
         1
@@ -236,7 +236,7 @@ test:do_execsql_test(
 test:do_execsql_test(
     3.8,
     [[
-        SELECT x FROM t2 WHERE '1.0' IN (x);
+        SELECT x FROM t2 WHERE '1' IN (x);
     ]], {
         -- <3.8>
         1

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-04-10 10:39     ` Mergen Imeev
@ 2020-04-10 10:43       ` Mergen Imeev
  2020-04-10 13:05         ` Nikita Pettik
  2020-04-10 12:46       ` Nikita Pettik
  1 sibling, 1 reply; 18+ messages in thread
From: Mergen Imeev @ 2020-04-10 10:43 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches

Sorry, forgot to answer one question.

On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote:
> Hi! Thank you for review. My answers and new patch below.
> 
> On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
> > On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > 
> > Could you please find Peter's table containing current/expected cast
> > behaviours and verify that this patch doesn't contradict it?
> > 
Here:
https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst

~                To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY
---------------  ----------   ----------   ---------   ---------   ------------
From BOOLEAN   | AAA        | A--        | ---       | A--       | ---
From INTEGER   | A--        | AAA        | AAA       | AAA       | ---
From NUMBER    | A--        | SSA        | AAA       | AAA       | ---
From STRING    | S--        | SSS        | SSS       | AAA       | A--
From VARBINARY | ---        | ---        | ---       | A--       | AAA

Should be fine since we have S for CAST() and implicit
cast from STRING to INTEGER.

> > > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > > index aad030d..de1d9c3 100644
> > > --- a/src/box/sql/vdbemem.c
> > > +++ b/src/box/sql/vdbemem.c
> > > @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> > >  		return -1;
> > >  	case FIELD_TYPE_INTEGER:
> > >  	case FIELD_TYPE_UNSIGNED:
> > > -		if ((pMem->flags & MEM_Blob) != 0) {
> > > +		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
> > >  			bool is_neg;
> > >  			int64_t val;
> > >  			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
> > > @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
> > >  			MemSetTypeFlag(pMem, MEM_UInt);
> > >  			return 0;
> > >  		}
> > > -		if (sqlVdbeMemIntegerify(pMem) != 0)
> > > +		if ((pMem->flags & MEM_Real) != 0) {
> > > +			double d;
> > > +			if (sqlVdbeRealValue(pMem, &d) != 0)
> > > +				return -1;
> > > +			if (d < INT64_MAX && d >= INT64_MIN) {
> > > +				mem_set_int(pMem, d, d <= -1);
> > > +				return 0;
> > > +			}
> > > +			if (d >= INT64_MAX && d < UINT64_MAX) {
> > > +				mem_set_u64(pMem, d);
> > > +				return 0;
> > > +			}
> > >  			return -1;
> > 
> > Instead of keeping inlining code into sqlVdbeMemCast() I'd better
> > split it into separate functions. Good refactoring task tho - just
> > keep in mind.
> > 
> Thanks. I will fix this in #3809 issue.
> 
> > > +		}
> > >  		if (type == FIELD_TYPE_UNSIGNED &&
> > >  		    (pMem->flags & MEM_UInt) == 0)
> > >  			return -1;
> > > diff --git a/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> > > new file mode 100755
> > > index 0000000..0865c4e
> > > --- /dev/null
> > > +++ b/test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> > 
> > Strictly speaking this change is not related to 4766, so I'd not
> > create separate test file for current patch. Moreover, tests in
> > sql/types.test.lua seem to cover it.
> > 
> I removed this file.
> 
> > > +#!/usr/bin/env tarantool
> > > +test = require("sqltester")
> > > +test:plan(6)
> > > +
> > > +--
> > > +-- Make sure that STRING or BLOB that contains DOUBLE value cannot
> > > +-- be cast to INTEGER.
> > > +--
> > > +test:do_catchsql_test(
> > > +    "gh-4766-1",
> > > +    [[
> > > +        SELECT CAST('1.1' AS INTEGER)
> > > +    ]], {
> > > +        1, "Type mismatch: can not convert 1.1 to integer"
> > > +    })
> > > +
> > > +test:do_catchsql_test(
> > > +    "gh-4766-2",
> > > +    [[
> > > +        SELECT CAST(x'312e31' AS INTEGER)
> > > +    ]], {
> > > +        1, "Type mismatch: can not convert varbinary to integer"
> > > +    })
> > > +
> > > diff --git a/test/sql/types.result b/test/sql/types.result
> > > index 38e4385..54aff46 100644
> > > --- a/test/sql/types.result
> > > +++ b/test/sql/types.result
> > > @@ -269,11 +269,8 @@ box.space.T1:drop()
> > >  --
> > >  box.execute("SELECT CAST('1.123' AS INTEGER);")
> > >  ---
> > > -- metadata:
> > > -  - name: CAST('1.123' AS INTEGER)
> > > -    type: integer
> > > -  rows:
> > > -  - [1]
> > > +- null
> > > +- 'Type mismatch: can not convert 1.123 to integer'
> > >  ...
> > >  box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
> > >  ---
> > > @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
> > >  ...
> > >  box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
> > >  ---
> > > -- metadata:
> > > -  - name: CAST(f AS INTEGER)
> > > -    type: integer
> > > -  rows:
> > > -  - [0]
> > > -  - [1]
> > > -  - [3]
> > > +- null
> > > +- 'Type mismatch: can not convert 0.0 to integer'
> > >  ...
> > >  box.space.T1:drop()
> > >  ---
> > > @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
> > >  ...
> > >  box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
> > >  ---
> > > -- null
> > > -- 'Type mismatch: can not convert -1 to unsigned'
> > > +- metadata:
> > > +  - name: CAST(-1.5 AS UNSIGNED)
> > > +    type: unsigned
> > > +  rows:
> > > +  - [-1]
> > >  ...
> > >  box.execute("SELECT CAST(true AS UNSIGNED);")
> > >  ---
> > > -- 
> > > 2.7.4
> > > 
> 
> 
> New patch:
> 
> 
> From d2679859651aeee2dffda01545e6c62ae3c185d1 Mon Sep 17 00:00:00 2001
> From: Mergen Imeev <imeevma@gmail.com>
> Date: Mon, 16 Mar 2020 15:12:37 +0300
> Subject: [PATCH] sql: fix CAST() from STRING to INTEGER
> 
> Prior to this patch, STRING, which contains the DOUBLE value,
> could be cast to INTEGER. This was done by converting STRING to
> DOUBLE and then converting this DOUBLE value to INTEGER. This may
> affect the accuracy of CAST(), so it was forbidden.
> 
> Before patch:
> box.execute("SELECT CAST('111.1' as INTEGER);")
> Result: 111
> 
> After patch:
> box.execute("SELECT CAST('1.1' as INTEGER);")
> Result: 'Type mismatch: can not convert 1.1 to integer'
> 
> box.execute("SELECT CAST('1.0' as INTEGER);")
> Result: 'Type mismatch: can not convert 1.0 to integer'
> 
> box.execute("SELECT CAST('1.' as INTEGER);")
> Result: 'Type mismatch: can not convert 1. to integer'
> 
> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index aad030d..de1d9c3 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
>  		return -1;
>  	case FIELD_TYPE_INTEGER:
>  	case FIELD_TYPE_UNSIGNED:
> -		if ((pMem->flags & MEM_Blob) != 0) {
> +		if ((pMem->flags & (MEM_Blob | MEM_Str)) != 0) {
>  			bool is_neg;
>  			int64_t val;
>  			if (sql_atoi64(pMem->z, &val, &is_neg, pMem->n) != 0)
> @@ -711,8 +711,20 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
>  			MemSetTypeFlag(pMem, MEM_UInt);
>  			return 0;
>  		}
> -		if (sqlVdbeMemIntegerify(pMem) != 0)
> +		if ((pMem->flags & MEM_Real) != 0) {
> +			double d;
> +			if (sqlVdbeRealValue(pMem, &d) != 0)
> +				return -1;
> +			if (d < INT64_MAX && d >= INT64_MIN) {
> +				mem_set_int(pMem, d, d <= -1);
> +				return 0;
> +			}
> +			if (d >= INT64_MAX && d < UINT64_MAX) {
> +				mem_set_u64(pMem, d);
> +				return 0;
> +			}
>  			return -1;
> +		}
>  		if (type == FIELD_TYPE_UNSIGNED &&
>  		    (pMem->flags & MEM_UInt) == 0)
>  			return -1;
> diff --git a/test/sql/types.result b/test/sql/types.result
> index 38e4385..54aff46 100644
> --- a/test/sql/types.result
> +++ b/test/sql/types.result
> @@ -269,11 +269,8 @@ box.space.T1:drop()
>  --
>  box.execute("SELECT CAST('1.123' AS INTEGER);")
>  ---
> -- metadata:
> -  - name: CAST('1.123' AS INTEGER)
> -    type: integer
> -  rows:
> -  - [1]
> +- null
> +- 'Type mismatch: can not convert 1.123 to integer'
>  ...
>  box.execute("CREATE TABLE t1 (f TEXT PRIMARY KEY);")
>  ---
> @@ -285,13 +282,8 @@ box.execute("INSERT INTO t1 VALUES('0.0'), ('1.5'), ('3.9312453');")
>  ...
>  box.execute("SELECT CAST(f AS INTEGER) FROM t1;")
>  ---
> -- metadata:
> -  - name: CAST(f AS INTEGER)
> -    type: integer
> -  rows:
> -  - [0]
> -  - [1]
> -  - [3]
> +- null
> +- 'Type mismatch: can not convert 0.0 to integer'
>  ...
>  box.space.T1:drop()
>  ---
> @@ -1105,8 +1097,11 @@ box.execute("SELECT CAST(1.5 AS UNSIGNED);")
>  ...
>  box.execute("SELECT CAST(-1.5 AS UNSIGNED);")
>  ---
> -- null
> -- 'Type mismatch: can not convert -1 to unsigned'
> +- metadata:
> +  - name: CAST(-1.5 AS UNSIGNED)
> +    type: unsigned
> +  rows:
> +  - [-1]
>  ...
>  box.execute("SELECT CAST(true AS UNSIGNED);")
>  ---

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-04-10 10:39     ` Mergen Imeev
  2020-04-10 10:43       ` Mergen Imeev
@ 2020-04-10 12:46       ` Nikita Pettik
  2020-04-10 13:05         ` Imeev Mergen
  1 sibling, 1 reply; 18+ messages in thread
From: Nikita Pettik @ 2020-04-10 12:46 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

On 10 Apr 13:39, Mergen Imeev wrote:
> Hi! Thank you for review. My answers and new patch below.
> 
> On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
> > On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > 
> > Could you please find Peter's table containing current/expected cast
> > behaviours and verify that this patch doesn't contradict it?

What about this?
 
> > > diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> > > index aad030d..de1d9c3 100644
> > > --- a/src/box/sql/vdbemem.c
> > > +++ b/src/box/sql/vdbemem.c
> > > @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER
  2020-04-10 10:41     ` Mergen Imeev
@ 2020-04-10 12:57       ` Nikita Pettik
  2020-04-10 18:09         ` Mergen Imeev
  0 siblings, 1 reply; 18+ messages in thread
From: Nikita Pettik @ 2020-04-10 12:57 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

On 10 Apr 13:41, Mergen Imeev wrote:
> On Fri, Mar 27, 2020 at 04:54:17PM +0000, Nikita Pettik wrote:
> > On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > > Prior to this patch, STRING, which contains the DOUBLE value,
> > > could be implicitly cast to INTEGER. This was done by converting
> > > STRING to DOUBLE and then converting this DOUBLE value to INTEGER.
> > > This may affect the accuracy of CAST(), so it was forbidden.
> > > 
> > > Example:
> > > box.execute("CREATE TABLE t(i INT PRIMARY KEY);")
> > > 
> > > Before patch:
> > > box.execute("INSERT INTO t VALUES ('111.1');")
> > > box.execute("SELECT * FROM t;")
> > > Result: 111
> > > 
> > > After patch:
> > > box.execute("INSERT INTO t VALUES ('1.1');")
> > > Result: 'Type mismatch: can not convert 1.1 to integer'
> > > 
> > > box.execute("INSERT INTO t VALUES ('1.0');")
> > > Result: 'Type mismatch: can not convert 1.0 to integer'
> > > 
> > > box.execute("INSERT INTO t VALUES ('1.');")
> > > Result: 'Type mismatch: can not convert 1. to integer'
> > 
> > Is comparison predicat affected?
> > 
> No, since all implicit casts in case of comparison executed
> inside of comparisons opcodes.

You should have mentioned this fact in commit message.

LGTM otherwise.
 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-04-10 10:43       ` Mergen Imeev
@ 2020-04-10 13:05         ` Nikita Pettik
  2020-04-10 17:06           ` Imeev Mergen
  0 siblings, 1 reply; 18+ messages in thread
From: Nikita Pettik @ 2020-04-10 13:05 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

On 10 Apr 13:43, Mergen Imeev wrote:
> Sorry, forgot to answer one question.
> 
> On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote:
> > Hi! Thank you for review. My answers and new patch below.
> > 
> > On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
> > > On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > > 
> > > Could you please find Peter's table containing current/expected cast
> > > behaviours and verify that this patch doesn't contradict it?
> > > 
> Here:
> https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst
> 
> ~                To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY
> ---------------  ----------   ----------   ---------   ---------   ------------
> From BOOLEAN   | AAA        | A--        | ---       | A--       | ---
> From INTEGER   | A--        | AAA        | AAA       | AAA       | ---
> From NUMBER    | A--        | SSA        | AAA       | AAA       | ---
> From STRING    | S--        | SSS        | SSS       | AAA       | A--
> From VARBINARY | ---        | ---        | ---       | A--       | AAA
> 
> Should be fine since we have S for CAST() and implicit
> cast from STRING to INTEGER.

This table describes current behaviour. Could you also find the table
which shows expected one?
 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-04-10 12:46       ` Nikita Pettik
@ 2020-04-10 13:05         ` Imeev Mergen
  0 siblings, 0 replies; 18+ messages in thread
From: Imeev Mergen @ 2020-04-10 13:05 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches


On 4/10/20 3:46 PM, Nikita Pettik wrote:
> On 10 Apr 13:39, Mergen Imeev wrote:
>> Hi! Thank you for review. My answers and new patch below.
>>
>> On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
>>> On 27 Mar 14:33, imeevma@tarantool.org wrote:
>>>
>>> Could you please find Peter's table containing current/expected cast
>>> behaviours and verify that this patch doesn't contradict it?
> What about this?

Answered in follow-up letter:

https://lists.tarantool.org/pipermail/tarantool-patches/2020-April/015650.html

>   
>>>> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
>>>> index aad030d..de1d9c3 100644
>>>> --- a/src/box/sql/vdbemem.c
>>>> +++ b/src/box/sql/vdbemem.c
>>>> @@ -696,7 +696,7 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-04-10 13:05         ` Nikita Pettik
@ 2020-04-10 17:06           ` Imeev Mergen
  2020-04-15 11:13             ` Nikita Pettik
  0 siblings, 1 reply; 18+ messages in thread
From: Imeev Mergen @ 2020-04-10 17:06 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches


On 4/10/20 4:05 PM, Nikita Pettik wrote:
> On 10 Apr 13:43, Mergen Imeev wrote:
>> Sorry, forgot to answer one question.
>>
>> On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote:
>>> Hi! Thank you for review. My answers and new patch below.
>>>
>>> On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
>>>> On 27 Mar 14:33, imeevma@tarantool.org wrote:
>>>>
>>>> Could you please find Peter's table containing current/expected cast
>>>> behaviours and verify that this patch doesn't contradict it?
>>>>
>> Here:
>> https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst
>>
>> ~                To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY
>> ---------------  ----------   ----------   ---------   ---------   ------------
>>  From BOOLEAN   | AAA        | A--        | ---       | A--       | ---
>>  From INTEGER   | A--        | AAA        | AAA       | AAA       | ---
>>  From NUMBER    | A--        | SSA        | AAA       | AAA       | ---
>>  From STRING    | S--        | SSS        | SSS       | AAA       | A--
>>  From VARBINARY | ---        | ---        | ---       | A--       | AAA
>>
>> Should be fine since we have S for CAST() and implicit
>> cast from STRING to INTEGER.
> This table describes current behaviour. Could you also find the table
> which shows expected one?
>   
I'm sorry, not sure that I have seen such table for CAST(). Should
I ask Peter?

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast from STRING to INTEGER
  2020-04-10 12:57       ` Nikita Pettik
@ 2020-04-10 18:09         ` Mergen Imeev
  0 siblings, 0 replies; 18+ messages in thread
From: Mergen Imeev @ 2020-04-10 18:09 UTC (permalink / raw)
  To: Nikita Pettik; +Cc: tarantool-patches

Hi! Thank you for the review. I extended commit-message.
You can see it below.

On Fri, Apr 10, 2020 at 12:57:57PM +0000, Nikita Pettik wrote:
> On 10 Apr 13:41, Mergen Imeev wrote:
> > On Fri, Mar 27, 2020 at 04:54:17PM +0000, Nikita Pettik wrote:
> > > On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > > > Prior to this patch, STRING, which contains the DOUBLE value,
> > > > could be implicitly cast to INTEGER. This was done by converting
> > > > STRING to DOUBLE and then converting this DOUBLE value to INTEGER.
> > > > This may affect the accuracy of CAST(), so it was forbidden.
> > > > 
> > > > Example:
> > > > box.execute("CREATE TABLE t(i INT PRIMARY KEY);")
> > > > 
> > > > Before patch:
> > > > box.execute("INSERT INTO t VALUES ('111.1');")
> > > > box.execute("SELECT * FROM t;")
> > > > Result: 111
> > > > 
> > > > After patch:
> > > > box.execute("INSERT INTO t VALUES ('1.1');")
> > > > Result: 'Type mismatch: can not convert 1.1 to integer'
> > > > 
> > > > box.execute("INSERT INTO t VALUES ('1.0');")
> > > > Result: 'Type mismatch: can not convert 1.0 to integer'
> > > > 
> > > > box.execute("INSERT INTO t VALUES ('1.');")
> > > > Result: 'Type mismatch: can not convert 1. to integer'
> > > 
> > > Is comparison predicat affected?
> > > 
> > No, since all implicit casts in case of comparison executed
> > inside of comparisons opcodes.
> 
> You should have mentioned this fact in commit message.
> 
Fixed:


sql: fix implicit cast from STRING to INTEGER

Prior to this patch, STRING, which contains the DOUBLE value,
could be implicitly cast to INTEGER. This was done by converting
STRING to DOUBLE and then converting this DOUBLE value to INTEGER.
This may affect the accuracy of CAST(), so it was forbidden. It
is worth noting that these changes will not affect the comparison,
since the implicit cast in this case has different mechanics.

Example:
box.execute("CREATE TABLE t(i INT PRIMARY KEY);")

Before patch:
box.execute("INSERT INTO t VALUES ('111.1');")
box.execute("SELECT * FROM t;")
Result: 111

After patch:
box.execute("INSERT INTO t VALUES ('1.1');")
Result: 'Type mismatch: can not convert 1.1 to integer'

box.execute("INSERT INTO t VALUES ('1.0');")
Result: 'Type mismatch: can not convert 1.0 to integer'

box.execute("INSERT INTO t VALUES ('1.');")
Result: 'Type mismatch: can not convert 1. to integer'

@TarantoolBot document
Title: disallow cast from STRING contains DOUBLE to INTEGER

After the last two patches, explicit and implicit casting from the
string containing DOUBLE to INTEGER directly will be prohibited.
The user must use the explicit cast to DOUBLE before the explicit
or implicit cast to INTEGER. The reason for this is that before
these patches, such STRINGs were implicitly cast to DOUBLE, and
then this DOUBLE was implicitly or explicitly cast to INTEGER.
Because of this, the result of such a cast may differ from what
the user expects, and the user may not know why.

It is worth noting that these changes will not affect the
comparison, since the implicit cast in this case has different
mechanics.

Example for implicit cast:

box.execute("CREATE TABLE t(i INT PRIMARY KEY);")
-- Does not work anymore:
box.execute("INSERT INTO t VALUES ('1.1');")
-- Right way:
box.execute("INSERT INTO t VALUES (CAST('1.1' AS DOUBLE));")

Example for explicit cast:

-- Does not work anymore:
box.execute("SELECT CAST('1.1' AS INTEGER);")
-- Right way:
box.execute("SELECT CAST(CAST('1.1' AS DOUBLE) AS INTEGER);")

> LGTM otherwise.
>  

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER
  2020-04-10 17:06           ` Imeev Mergen
@ 2020-04-15 11:13             ` Nikita Pettik
  0 siblings, 0 replies; 18+ messages in thread
From: Nikita Pettik @ 2020-04-15 11:13 UTC (permalink / raw)
  To: Imeev Mergen; +Cc: tarantool-patches

On 10 Apr 20:06, Imeev Mergen wrote:
> 
> On 4/10/20 4:05 PM, Nikita Pettik wrote:
> > On 10 Apr 13:43, Mergen Imeev wrote:
> > > Sorry, forgot to answer one question.
> > > 
> > > On Fri, Apr 10, 2020 at 01:39:45PM +0300, Mergen Imeev wrote:
> > > > Hi! Thank you for review. My answers and new patch below.
> > > > 
> > > > On Fri, Mar 27, 2020 at 04:46:04PM +0000, Nikita Pettik wrote:
> > > > > On 27 Mar 14:33, imeevma@tarantool.org wrote:
> > > > > 
> > > > > Could you please find Peter's table containing current/expected cast
> > > > > behaviours and verify that this patch doesn't contradict it?
> > > > > 
> > > Here:
> > > https://github.com/tarantool/doc/blob/pgulutzan-2.3/doc/reference/reference_sql/sql.rst
> > > 
> > > ~                To BOOLEAN | To INTEGER | To NUMBER | To STRING | To VARBINARY
> > > ---------------  ----------   ----------   ---------   ---------   ------------
> > >  From BOOLEAN   | AAA        | A--        | ---       | A--       | ---
> > >  From INTEGER   | A--        | AAA        | AAA       | AAA       | ---
> > >  From NUMBER    | A--        | SSA        | AAA       | AAA       | ---
> > >  From STRING    | S--        | SSS        | SSS       | AAA       | A--
> > >  From VARBINARY | ---        | ---        | ---       | A--       | AAA
> > > 
> > > Should be fine since we have S for CAST() and implicit
> > > cast from STRING to INTEGER.
> > This table describes current behaviour. Could you also find the table
> > which shows expected one?
> I'm sorry, not sure that I have seen such table for CAST(). Should
> I ask Peter?

I did it myself: thread is called "cast" in obsolete dev@tarantool mailing
list (15 May 2019 12:44:54). There's even corresponding issue which was
closed by Kirill for some reason:
https://github.com/tarantool/tarantool/issues/4216

Now I am okay with these patches.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER
  2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma
                   ` (2 preceding siblings ...)
  2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma
@ 2020-04-16  0:03 ` Nikita Pettik
  3 siblings, 0 replies; 18+ messages in thread
From: Nikita Pettik @ 2020-04-16  0:03 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

On 27 Mar 14:33, imeevma@tarantool.org wrote:
> This patch-set fixes CAST() from BLOB to INTEGER in case a BLOB
> does not have '\0'.
> 
> https://github.com/tarantool/tarantool/issues/4766
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4766-fix-blob-size-for-cast
> 
> @Changelog
>  - Explicit and implicit cast from string contains DOUBLE value to
>    INTEGER or UNSIGNED was disallowed.
>  - Maximum length of a BLOB that is allowed to be cast to INTEGER
>    or UNSIGNED was limited to 12287 bytes.
>  - Fixed wrong behaviour of CAST() from BLOB to INTEGER in case a
>    BLOB does not have '\0'. (gh-4766)

Pushed to master, updated changelog, dropped branch.
 
> Mergen Imeev (3):
>   sql: fix CAST() from STRING to INTEGER
>   sql: fix implicit cast from STRING to INTEGER
>   sql: add '\0' to the BLOB when it is cast to INTEGER
> 
>  src/box/sql/util.c                                 |  17 ++-
>  src/box/sql/vdbe.c                                 |  11 +-
>  src/box/sql/vdbeInt.h                              |   1 -
>  src/box/sql/vdbemem.c                              |  45 ++-----
>  test/sql-tap/e_select1.test.lua                    |   2 +-
>  .../gh-4766-wrong-cast-from-blob-to-int.test.lua   | 150 +++++++++++++++++++++
>  test/sql-tap/intpkey.test.lua                      |   2 +-
>  test/sql-tap/join.test.lua                         |   4 +-
>  test/sql-tap/subquery.test.lua                     |   6 +-
>  test/sql-tap/tkt-9a8b09f8e6.test.lua               |   4 +-
>  test/sql/types.result                              |  23 ++--
>  11 files changed, 203 insertions(+), 62 deletions(-)
>  create mode 100755 test/sql-tap/gh-4766-wrong-cast-from-blob-to-int.test.lua
> 
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-04-16  0:03 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-27 11:33 [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB to INTEGER imeevma
2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING " imeevma
2020-03-27 16:46   ` Nikita Pettik
2020-04-10 10:39     ` Mergen Imeev
2020-04-10 10:43       ` Mergen Imeev
2020-04-10 13:05         ` Nikita Pettik
2020-04-10 17:06           ` Imeev Mergen
2020-04-15 11:13             ` Nikita Pettik
2020-04-10 12:46       ` Nikita Pettik
2020-04-10 13:05         ` Imeev Mergen
2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 2/3] sql: fix implicit cast " imeevma
2020-03-27 16:54   ` Nikita Pettik
2020-04-10 10:41     ` Mergen Imeev
2020-04-10 12:57       ` Nikita Pettik
2020-04-10 18:09         ` Mergen Imeev
2020-03-27 11:33 ` [Tarantool-patches] [PATCH v4 3/3] sql: add '\0' to the BLOB when it is cast " imeevma
2020-03-27 16:54   ` Nikita Pettik
2020-04-16  0:03 ` [Tarantool-patches] [PATCH v4 0/3] sql: fix CAST() from BLOB " Nikita Pettik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox