[tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 10 comments, review fixes on the
branch and here.

Also, please, elaborate what is wrong with tests - they do
not pass.

On 12/10/2018 14:19, n.pettik wrote:
> 
>>> From: Georgy Kirichenko <georgy@tarantool.org>
>>> Most DBs (at least PostgreSQL, Oracle and DB2) allow to process
>>> following queries:
>>> CREATE TABLE t1 (id INT PRIMARY KEY);
>>> INSERT INTO t1 VALUES (1.123), ('2');
>>> In this particular case, 1.123 should be simply truncated to 1,
>>> and '2' - converted to literal number 2.
>>> After passing real type to Tarantool (instead of <SCALAR>), example
>>> above would fail without conversions. Thus, lets add implicit
>>> conversions inside VDBE to make this example be legal.
>>> However, still some types conversions must be prohibited. For instance,
>>> <BLOB> can't be converted to integer or floating point numerical,
>>> and vice versa.
>>
>> As I see in the tests that it looks weird now:
>>
>> I can insert into 'int' a 'float' value, but can not
>> compare them:
>>
>>     box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<2.001")
>>     ---
>>     -- error: Can't convert 2.001 to INTEGER
>>     ...
>>
>> Why? We should either forbid insertion and comparison, or
>> allow both of them.
> 
> Well, now I agree that it looks quite strange, but I can't tell you
> why I did so. It took quite a long to fix that, but workaround turned
> out the be trivial:
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 3d2324867..827811cd1 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -335,6 +335,14 @@ mem_apply_affinity(struct Mem *record, enum affinity_type affinity)
>          case AFFINITY_INTEGER:
>                  if ((record->flags & MEM_Int) == MEM_Int)
>                          return 0;
> +               if ((record->flags & MEM_Real) == MEM_Real) {
> +                       int64_t i = (int64_t) record->u.r;
> +                       if (i == record->u.r) {
> +                               record->u.i = i;
> +                               MemSetTypeFlag(record, MEM_Int);
> +                       }
> +                       return 0;
> +               }
>                  return sqlite3VdbeMemIntegerify(record, false);
>          case AFFINITY_REAL:
>                  if ((record->flags & MEM_Real) == MEM_Real)
> @@ -1918,6 +1926,13 @@ case OP_MustBeInt: {            /* jump, in1 */
>          pIn1 = &aMem[pOp->p1];
>          if ((pIn1->flags & MEM_Int)==0) {
>                  mem_apply_affinity(pIn1, AFFINITY_INTEGER);
> +               if ((pIn1->flags & MEM_Real) == MEM_Real) {
> +                       int64_t i = (int64_t) pIn1->u.r;
> +                       if (i == pIn1->u.r) {
> +                               pIn1->u.i = i;
> +                               MemSetTypeFlag(pIn1, MEM_Int);
> +                       }
> +               }

1. Why? mem_apply_affinity does the same and is called one line
above, it is not?

>                  VdbeBranchTaken((pIn1->flags&MEM_Int)==0, 2);
>                  if ((pIn1->flags & MEM_Int)==0) {
>                          if (pOp->p2==0) {
> @@ -3463,7 +3478,6 @@ case OP_SeekGT: {       /* jump, in3 */
>          reg_ipk = pOp->p5;
>   
>          if (reg_ipk > 0) {
> -
>                  /* The input value in P3 might be of any type: integer, real, string,
>                   * blob, or NULL.  But it needs to be an integer before we can do
>                   * the seek, so convert it.
> @@ -3473,9 +3487,18 @@ case OP_SeekGT: {       /* jump, in3 */
>                          applyNumericAffinity(pIn3, 0);
>                  }
>                  int64_t i;
> -               if (sqlite3VdbeIntValue(pIn3, &i) != 0) {
> +               if ((pIn3->flags & MEM_Int) == MEM_Int) {
> +                       i = pIn3->u.i;
> +               } else if ((pIn3->flags & MEM_Real) == MEM_Real) {
> +                       if (pIn3->u.r > INT64_MAX)
> +                               i = INT64_MAX;
> +                       else if (pIn3->u.r < INT64_MIN)
> +                               i = INT64_MIN;

2. In the explanation below you say 'without loses' but here
they are, it is not? An example: I do search by SeekLT with
something bigger than INT64_MAX and the index really has
INT64_MAX. You will turn this query into SeekLT by INT64_MAX,
so INT64_MAX will not be presented in the result, but should be.
I guess, if I am right, you can fix it easy just changing
OP_SeekLT to OP_SeekLE in such a case, and the same for
OP_SeekGT/GE and INT64_MIN.

> +                       else
> +                               i = pIn3->u.r;
> +               } else {
> *Explanation*
> 
> Now OP_Affinity doesn’t ‘integrify’ float since it leads to losing
> information concerning the fact that initial ‘real’ value was greater or less
> than truncated ‘int’. Instead, it is done by explicit OP_MustBeInt conversion
> OR when we are already in comparing routine and do it implicitly.
> The only one case when OP_Affinity changes float to int is situation
> when float can be casted to int without loses.
> 
>>> +
>>> +		sqlite3VdbeError(p, format, sqlite3_value_text(pIn1));
>>> +		goto abort_due_to_error;
>>> +	}
>>>   	break;
>>>   }
>>>   #endif /* SQLITE_OMIT_CAST */
>>> @@ -2451,11 +2516,13 @@ case OP_IfNot: {            /* jump, in1 */
>>>   	if (pIn1->flags & MEM_Null) {
>>>   		c = pOp->p3;
>>>   	} else {
>>> -#ifdef SQLITE_OMIT_FLOATING_POINT
>>> -		c = sqlite3VdbeIntValue(pIn1)!=0;
>>> -#else
>>> -		c = sqlite3VdbeRealValue(pIn1)!=0.0;
>>> -#endif
>>> +		double v;
>>> +		if ((rc = sqlite3VdbeRealValue(pIn1, &v))) {
>>> +			sqlite3VdbeError(p, "Can't convert to numeric %s",
>>
>> 3. Why numeric? Maybe real?
> 
> It doesn’t really matter. Currently they are the same.

3. I know, but when a user asks to cast to real or just operates
on real, and gets an error message "Can't convert to numeric",
it is strange.

> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index 072a05066..f9527b650 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
>   /*
>    * Convert pMem to type integer.  Invalidate any prior representations.
>    */
>   int
> -sqlite3VdbeMemIntegerify(Mem * pMem)
> +sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced)
>   {
>   	assert(EIGHT_BYTE_ALIGNMENT(pMem));
>   
> -	pMem->u.i = sqlite3VdbeIntValue(pMem);
> +	int64_t i;
> +	if (sqlite3VdbeIntValue(pMem, &i) == 0) {
> +		pMem->u.i = i;
> +		MemSetTypeFlag(pMem, MEM_Int);
> +		return SQLITE_OK;
> +	} else if ((pMem->flags & MEM_Real) != 0 && is_forced) {
> +		pMem->u.i = (int) pMem->u.r;
> +		MemSetTypeFlag(pMem, MEM_Int);
> +		return 0;

4. Lets do not mix SQLITE_OK/0 in one function.

> +	}
> +
> +	double d;
> +	if (sqlite3VdbeRealValue(pMem, &d) || (int64_t) d != d) {
> +		return SQLITE_ERROR;
> +	}
> +	pMem->u.i = (int64_t) d;
>   	MemSetTypeFlag(pMem, MEM_Int);
>   	return SQLITE_OK;
>   }
> @@ -583,46 +601,47 @@ sqlite3VdbeMemNumerify(Mem * pMem)
>    * affinity even if that results in loss of data.  This routine is
>    * used (for example) to implement the SQL "cast()" operator.
>    */
> -void
> +int
>   sqlite3VdbeMemCast(Mem * pMem, u8 aff)
>   {
>   	if (pMem->flags & MEM_Null)
> -		return;
> -	switch (aff) {
> -	case AFFINITY_BLOB:{	/* Really a cast to BLOB */
> -			if ((pMem->flags & MEM_Blob) == 0) {
> -				sqlite3ValueApplyAffinity(pMem, AFFINITY_TEXT);
> -				assert(pMem->flags & MEM_Str
> -				       || pMem->db->mallocFailed);
> -				if (pMem->flags & MEM_Str)
> -					MemSetTypeFlag(pMem, MEM_Blob);
> -			} else {
> -				pMem->flags &= ~(MEM_TypeMask & ~MEM_Blob);
> -			}
> -			break;
> -		}
> -	case AFFINITY_NUMERIC:{
> -			sqlite3VdbeMemNumerify(pMem);
> -			break;
> -		}
> -	case AFFINITY_INTEGER:{
> -			sqlite3VdbeMemIntegerify(pMem);
> -			break;
> -		}
> -	case AFFINITY_REAL:{
> -			sqlite3VdbeMemRealify(pMem);
> -			break;
> +		return SQLITE_OK;
> +	if (pMem->flags & MEM_Blob && aff == AFFINITY_INTEGER)
> +		return sql_atoi64(pMem->z, &pMem->u.i, pMem->n);
> +	if (pMem->flags & MEM_Blob &&
> +	    (aff == AFFINITY_REAL || aff == AFFINITY_NUMERIC)) {
> +		if (sql_atoi64(pMem->z, &pMem->u.i, pMem->n) == 0) {
> +			MemSetTypeFlag(pMem, MEM_Real);
> +			pMem->u.r = pMem->u.i;
> +			return 0;
>   		}

5. I guess, you can move these two checks down to the switch into
corresponding cases to avoid unnecessary checks. See my review fixes.
And if you applied them, I would answer why not to fix
sqlite3VdbeIntValue to allow cast BLOB to int as you do here out
of sqlite3VdbeIntValue? I mean to delete this 'if' above:

	if (pMem->flags & MEM_Blob && aff == AFFINITY_INTEGER)

and allow 'case AFFINITY_INTEGER:' do its work below.

I did not move this case:

	if ((pMem->flags & MEM_Blob) != 0 &&
	    (aff == AFFINITY_REAL || aff == AFFINITY_NUMERIC)) {

Please, do it by yourself.

> diff --git a/test/sql-tap/boundary2.test.lua b/test/sql-tap/boundary2.test.lua
> index 3eaef75dc..be4b8750d 100755
> --- a/test/sql-tap/boundary2.test.lua
> +++ b/test/sql-tap/boundary2.test.lua
> @@ -1,6 +1,6 @@
>   #!/usr/bin/env tarantool
>   test = require("sqltester")
> -test:plan(3021)
> +test:plan(2965)
>   
>   --!./tcltestrunner.lua
>   -- 2008 December 11
> @@ -7462,6 +7462,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE r > 9.22337303685477580800e+18 ORDER BY a DESC",
>       {})
>   
> +if false then

6. I thought you have removed all these 'if false'. Please, do it.

>   test:do_execsql_test(
>       "boundary2-2.65.gt.3",
>       "SELECT a FROM t1 WHERE r > 9.22337303685477580800e+18 ORDER BY r",
> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
> index eb4f43a90..e5a2d7a5a 100755
> --- a/test/sql-tap/collation.test.lua
> +++ b/test/sql-tap/collation.test.lua
> @@ -250,8 +250,8 @@ local like_testcases =
>   test:do_catchsql_set_test(like_testcases, prefix)
>   
>   test:do_catchsql_test(
> -        "collation-2.5.0",
> -        'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
> -        {1, "Collation 'FOO' does not exist"})
> +    "collation-2.5.0",
> +    'CREATE TABLE test3 (a int, b int, c int, PRIMARY KEY (a, a COLLATE foo, b, c))',
> +    {1, "Collation 'FOO' does not exist"})

7. Nothing has changed. Stray diff.

>   
>   test:finish_test()
> diff --git a/test/sql-tap/like3.test.lua b/test/sql-tap/like3.test.lua
> index ea6824ba7..a683df2d6 100755
> --- a/test/sql-tap/like3.test.lua
> +++ b/test/sql-tap/like3.test.lua
> @@ -75,9 +75,9 @@ test:do_execsql_test(
>           CREATE INDEX t2ba ON t2(b,a);
>           SELECT a, b FROM t2 WHERE b GLOB 'ab*' ORDER BY +a;
>       ]], {
> -        -- <like3-2.0>
> -        1, "abc", 4, "abc"
> -        -- </like3-2.0>
> +    -- <like3-2.0>
> +    1, "abc", 4, "abc"
> +     -- </like3-2.0>

8. The same, and below.

>       })
>   
>   test:do_execsql_test(
> @@ -85,54 +85,55 @@ test:do_execsql_test(
>       [[
>           SELECT a, b FROM t2 WHERE +b GLOB 'ab*' ORDER BY +a;
>       ]], {
> -        -- <like3-2.1>
> -        1, "abc", 4, "abc"
> -        -- </like3-2.1>
> -    })
> +    -- <like3-2.1>
> +    1, "abc", 4, "abc"
> +    -- </like3-2.1>
> +     })
>   
>   test:execsql([[
>       CREATE TABLE t3(x TEXT PRIMARY KEY COLLATE "unicode_ci");
>       INSERT INTO t3(x) VALUES('aaa'),('abc'),('abd'),('abe'),('acz');
> -    INSERT INTO t3(x) SELECT CAST(x AS blob) FROM t3;
> +--    INSERT INTO t3(x) SELECT CAST(x AS blob) FROM t3;

9. Delete or uncomment, please.

>   ]])
>   
>   -- MUST_WORK #1476 collate nocase
> diff --git a/test/sql-tap/numcast.test.lua b/test/sql-tap/numcast.test.lua
> index f917e5a51..6750cefef 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(17)
> +test:plan(9)
>   
>   --!./tcltestrunner.lua
>   -- 2013 March 20
> @@ -38,11 +38,8 @@ for _, enc in ipairs({"utf8"}) do
>           {"1", "12345.0", 12345.0, 12345},
>           {"2", "12345.0e0", 12345.0, 12345},
>           {"3", "-12345.0e0", -12345.0, -12345},
> -        {"4", "-12345.25", -12345.25, -12345},
> +--        {"4", "-12345.25", -12345.25, -12345},

10. --

>           {"5", "-12345.0", -12345.0, -12345},
> -        {"6", "'876xyz'", 876.0, 876},
> -        {"7", "'456ķ89'", 456.0, 456},
> -        {"8", "'Ġ 321.5'", 0.0, 0},
>       }
>       for _, val in ipairs(data) do
>           local idx = val[1]
Review fixes:

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

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 55bdeb4e3..5ad643f25 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -283,14 +283,10 @@ sqlite3VdbeMemStringify(Mem * pMem, u8 bForce)
  	int fg = pMem->flags;
  	const int nByte = 32;
  
-	if (fg & MEM_Null)
-		return SQLITE_OK;
-
-	if (fg & (MEM_Str | MEM_Blob))
+	if ((fg & (MEM_Null | MEM_Str | MEM_Blob)) != 0)
  		return SQLITE_OK;
  
  	assert(!(fg & MEM_Zero));
-	assert(!(fg & (MEM_Str | MEM_Blob)));
  	assert(fg & (MEM_Int | MEM_Real));
  	assert(EIGHT_BYTE_ALIGNMENT(pMem));
  
@@ -532,7 +528,7 @@ sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced)
  	if (sqlite3VdbeIntValue(pMem, &i) == 0) {
  		pMem->u.i = i;
  		MemSetTypeFlag(pMem, MEM_Int);
-		return SQLITE_OK;
+		return 0;
  	} else if ((pMem->flags & MEM_Real) != 0 && is_forced) {
  		pMem->u.i = (int) pMem->u.r;
  		MemSetTypeFlag(pMem, MEM_Int);
@@ -545,7 +541,7 @@ sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced)
  	}
  	pMem->u.i = (int64_t) d;
  	MemSetTypeFlag(pMem, MEM_Int);
-	return SQLITE_OK;
+	return 0;
  }
  
  /*
@@ -606,9 +602,7 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff)
  {
  	if (pMem->flags & MEM_Null)
  		return SQLITE_OK;
-	if (pMem->flags & MEM_Blob && aff == AFFINITY_INTEGER)
-		return sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, pMem->n);
-	if (pMem->flags & MEM_Blob &&
+	if ((pMem->flags & MEM_Blob) != 0 &&
  	    (aff == AFFINITY_REAL || aff == AFFINITY_NUMERIC)) {
  		if (sql_atoi64(pMem->z, (int64_t *) &pMem->u.i, pMem->n) == 0) {
  			MemSetTypeFlag(pMem, MEM_Real);
@@ -631,6 +625,10 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff)
  	case AFFINITY_NUMERIC:
  		return sqlite3VdbeMemNumerify(pMem);
  	case AFFINITY_INTEGER:
+		if ((pMem->flags & MEM_Blob) != 0) {
+			return sql_atoi64(pMem->z, (int64_t *) &pMem->u.i,
+					  pMem->n);
+		}
  		return sqlite3VdbeMemIntegerify(pMem, true);
  	case AFFINITY_REAL:
  		return sqlite3VdbeMemRealify(pMem);