[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type

["n.pettik" <korablev@tarantool.org>]

> -------------------------
> vdbemem.c:661
> 
>> 	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);
>> 			pMem->u.r = pMem->u.i;
>> 			return 0;
>> 		}
>> 		return ! sqlAtoF(pMem->z, &pMem->u.r, pMem->n);
>> 	}
> 
> Here you use assign 'pMem.u.i' to 'pMem.u.r' without checking
> its sign. In theory something like that should work wrong now:
> 
>     box.execute("SELECT CAST('9223372036854775837' AS REAL)")
> 
> But you need to somehow write that number as blob.

Thanks, you are absolutely right:

tarantool> SELECT CAST(x'3138343436373434303733373039353531363135' AS real)
---
- metadata:
  - name: CAST(x'3138343436373434303733373039353531363135' AS real)
    type: number
  rows:
  - [-1]
...

Fix:

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 64acb5d41..087cd20f1 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -658,7 +658,10 @@ sqlVdbeMemCast(Mem * pMem, enum field_type type)
                if (sql_atoi64(pMem->z,  (int64_t *) &pMem->u.i, &is_neg,
                               pMem->n) == 0) {
                        MemSetTypeFlag(pMem, MEM_Real);
-                       pMem->u.r = pMem->u.i;
+                       if (is_neg)
+                               pMem->u.r = pMem->u.i;
+                       else
+                               pMem->u.r = pMem->u.u;
                        return 0;
                }
                return ! sqlAtoF(pMem->z, &pMem->u.r, pMem->n);
diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
index 315f49d08..3027657a5 100755
--- a/test/sql-tap/cast.test.lua
+++ b/test/sql-tap/cast.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(82)
+test:plan(84)
 
 --!./tcltestrunner.lua
 -- 2005 June 25
@@ -793,6 +793,18 @@ if true then --test:execsql("PRAGMA encoding")[1][1]=="UTF-8" then
         })
 end
 
+test:do_execsql_test(
+    "case-3.25",
+    [[
+        SELECT CAST(x'3138343436373434303733373039353531363135' AS REAL);
+    ]], { 1.844674407371e+19 } )
+
+test:do_execsql_test(
+    "case-3.26",
+    [[
+        SELECT CAST(x'3138343436373434303733373039353531363135' AS INT);
+    ]], { 18446744073709551615LL } )
+
 test:do_execsql_test(
     "case-3.31",
     [[

> -------------------------
> vdbetrace.c:90
> 
> Function sqlVdbeExpandSql stringifies integers, but
> misses a version for big unsigned.

diff --git a/src/box/sql/vdbetrace.c b/src/box/sql/vdbetrace.c
index f78d5b093..2ee9f668c 100644
--- a/src/box/sql/vdbetrace.c
+++ b/src/box/sql/vdbetrace.c
@@ -151,6 +151,8 @@ sqlVdbeExpandSql(Vdbe * p,  /* The prepared statement being evaluated */
                                sqlStrAccumAppend(&out, "NULL", 4);
                        } else if (pVar->flags & MEM_Int) {
                                sqlXPrintf(&out, "%lld", pVar->u.i);
+                       } else if (pVar->flags & MEM_UInt) {
+                               sqlXPrintf(&out, "%llu", pVar->u.u);
                        } else if (pVar->flags & MEM_Real) {
                                sqlXPrintf(&out, "%!.15g", pVar->u.r);
                        } else if (pVar->flags & MEM_Str) {

> -------------------------
> vdbe.c:307
> 
>> 	case FIELD_TYPE_INTEGER:
>> 	case FIELD_TYPE_UNSIGNED:
>> 		if ((record->flags & MEM_Int) == MEM_Int)
>> 			return 0;
>> 		if ((record->flags & MEM_UInt) == MEM_UInt)
>> 			return 0;
>> 		if ((record->flags & MEM_Real) == MEM_Real) {
>> 			int64_t i = (int64_t) record->u.r;
>> 			if (i == record->u.r)
>> 				mem_set_int(record, record->u.r,
>> 					    record->u.r <= -1);
>> 			return 0;
>> 		}
> 
> It is a part of function mem_apply_type. When target type is
> UNSIGNED, and a value is MEM_Int, you do nothing. Why? Looks like
> it is possible to pass here a negative value, and CAST UNSIGNED
> would do nothing.

Basically, this function implements sort of implicit cast
which takes place before comparison/assignment.
For comparisons it makes no sense - we can compare
integer with unsigned value - the latter is always greater.
For assignment it is also meaningless: if we attempt
at inserting negative values to unsigned field appropriate
error will be raised anyway. If you can come up with
specific example, let’s discuss it.