Re: [Tarantool-patches] [PATCH v4 1/3] sql: fix CAST() from STRING to INTEGER

[Mergen Imeev <imeevma@tarantool.org>]

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);")
 ---