Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: tarantool-patches@freelists.org, Nikita Pettik <korablev@tarantool.org>
Cc: Georgy Kirichenko <georgy@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions
Date: Thu, 27 Sep 2018 23:24:05 +0300	[thread overview]
Message-ID: <3ba8b43c-c6d5-ff34-1fd2-c302eae1a760@tarantool.org> (raw)
In-Reply-To: <d060ef561a49722c55bff0ead926f9642df98e54.1537216078.git.korablev@tarantool.org>

See 14 comments below, my review fixes on the branch and at
the end of the email.

On 17/09/2018 23:32, Nikita 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.

> ---
>   src/box/sql/expr.c                   |  17 +--
>   src/box/sql/insert.c                 |  14 ++-
>   src/box/sql/vdbe.c                   | 195 +++++++++++++++++++++++++----------
>   src/box/sql/vdbeInt.h                |  10 +-
>   src/box/sql/vdbeapi.c                |  12 ++-
>   src/box/sql/vdbemem.c                | 178 ++++++++++++++++++--------------
>   test/sql-tap/analyze9.test.lua       |   2 +-
>   test/sql-tap/autoinc.test.lua        |   2 +-
>   test/sql-tap/badutf1.test.lua        |  23 +----
>   test/sql-tap/boundary1.test.lua      |  19 +++-
>   test/sql-tap/boundary2.test.lua      |  20 +++-
>   test/sql-tap/cast.test.lua           |  25 ++++-
>   test/sql-tap/collation.test.lua      |   7 +-
>   test/sql-tap/e_select1.test.lua      |   4 +-
>   test/sql-tap/fkey2.test.lua          |   2 +-
>   test/sql-tap/in3.test.lua            |   5 +-
>   test/sql-tap/insert3.test.lua        |  40 +++----
>   test/sql-tap/like3.test.lua          |  66 +-----------
>   test/sql-tap/misc1.test.lua          |  13 +--
>   test/sql-tap/numcast.test.lua        |   7 +-
>   test/sql-tap/quote.test.lua          |  12 +--
>   test/sql-tap/selectA.test.lua        |  20 ++--
>   test/sql-tap/sort.test.lua           |  22 +---
>   test/sql-tap/tkt-a8a0d2996a.test.lua | 103 +-----------------
>   test/sql-tap/types2.test.lua         |   4 +-
>   test/sql/on-conflict.test.lua        |   4 +
>   test/sql/persistency.result          |   5 +-
>   test/sql/transition.result           |   3 +-
>   test/sql/triggers.result             |  12 +--
>   test/sql/triggers.test.lua           |  12 +--
>   30 files changed, 404 insertions(+), 454 deletions(-)
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 881666db3..b1d48ff72 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1945,11 +1970,27 @@ case OP_Cast: {                  /* in1 */
>   	testcase( pOp->p2==AFFINITY_INTEGER);
>   	testcase( pOp->p2==AFFINITY_REAL);
>   	pIn1 = &aMem[pOp->p1];
> -	memAboutToChange(p, pIn1);
>   	rc = ExpandBlob(pIn1);
> -	sqlite3VdbeMemCast(pIn1, pOp->p2);
> +	if (rc)
> +		goto abort_due_to_error;
> +	rc = sqlite3VdbeMemCast(pIn1, pOp->p2);
>   	UPDATE_MAX_BLOBSIZE(pIn1);
> -	if (rc) goto abort_due_to_error;
> +	if (rc) {
> +		const char *format;
> +		if (pOp->p2 == AFFINITY_TEXT)
> +			format = "Can't convert %s to TEXT";
> +		else if (pOp->p2 == AFFINITY_BLOB)
> +			format = "Can't convert %s to BLOB";
> +		else if (pOp->p2 == AFFINITY_NUMERIC)
> +			format = "Can't convert %s to NUMERIC";
> +		else if (pOp->p2 == AFFINITY_INTEGER)
> +			format = "Can't convert %s to INTEGER";
> +		else if (pOp->p2 == AFFINITY_REAL)
> +			format = "Can't convert %s to REAL";

1. Either we report uppercase affinity in all error messages,
or in no one. I fixed to lowercase.

2. The same about format. Here you report first value and second
type. In other places vice versa. I think, we should create a
separate error code for this and use diag_set here and in
other places.

> +
> +		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?

> +					 sqlite3_value_text(pIn1));
> +			goto abort_due_to_error;
> +		}
> +		c = v != 0;
>   		if (pOp->opcode==OP_IfNot) c = !c;
>   	}
>   	VdbeBranchTaken(c!=0, 2);
> diff --git a/test/sql-tap/boundary1.test.lua b/test/sql-tap/boundary1.test.lua
> index e35e1edbd..d149008b1 100755
> --- a/test/sql-tap/boundary1.test.lua
> +++ b/test/sql-tap/boundary1.test.lua
> @@ -7460,6 +7460,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE rowid > 9.22337303685477580800e+18 ORDER BY a DESC",
>       {})
>   
> +if false then

4. Why the test is commented? If it does not pass, then
uncomment and check an error message. I did it just to check
what is a problem and found "Can't convert 9.22337303685478e+18 to numeric".
Why I can not convert? - it looks like a valid number.

The same below for each 'if false', including boundary2.test.lua.

>   test:do_execsql_test(
>       "boundary1-2.65.gt.3",
>       "SELECT a FROM t1 WHERE rowid > 9.22337303685477580800e+18 ORDER BY rowid",
> @@ -7485,6 +7487,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE rowid >= 9.22337303685477580800e+18 ORDER BY a DESC",
>       {})
>   
> +if false then
>   test:do_execsql_test(
>       "boundary1-2.65.ge.3",
>       "SELECT a FROM t1 WHERE rowid >= 9.22337303685477580800e+18 ORDER BY rowid",
> @@ -7510,6 +7514,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE rowid < 9.22337303685477580800e+18 ORDER BY a DESC",
>       {64, 63, 62, 61, 60, 59, 58, 57, 56, 55, 54, 53, 52, 51, 50, 49, 48, 47, 46, 45, 44, 43, 42, 41, 40, 39, 38, 37, 36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1})
>   
> +if false then
>   test:do_execsql_test(
>       "boundary1-2.65.lt.3",
>       "SELECT a FROM t1 WHERE rowid < 9.22337303685477580800e+18 ORDER BY rowid",
> @@ -7535,6 +7541,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE rowid <= 9.22337303685477580800e+18 ORDER BY a DESC",
>       {64, 63, 62, 61, 60, 59, 58, 57, 56, 55, 54, 53, 52, 51, 50, 49, 48, 47, 46, 45, 44, 43, 42, 41, 40, 39, 38, 37, 36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1})
>   
> +if false then
>   test:do_execsql_test(
>       "boundary1-2.65.le.3",
>       "SELECT a FROM t1 WHERE rowid <= 9.22337303685477580800e+18 ORDER BY rowid",
>       "SELECT a FROM t1 WHERE rowid > -9.22337303685477580800e+18 ORDER BY a DESC",
>       {64, 63, 62, 61, 60, 59, 58, 57, 56, 55, 54, 53, 52, 51, 50, 49, 48, 47, 46, 45, 44, 43, 42, 41, 40, 39, 38, 37, 36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1})
>   
> +if false then
>   test:do_execsql_test(
>       "boundary1-2.66.gt.3",
>       "SELECT a FROM t1 WHERE rowid > -9.22337303685477580800e+18 ORDER BY rowid",
> @@ -7585,6 +7595,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE rowid >= -9.22337303685477580800e+18 ORDER BY a DESC",
>       {64, 63, 62, 61, 60, 59, 58, 57, 56, 55, 54, 53, 52, 51, 50, 49, 48, 47, 46, 45, 44, 43, 42, 41, 40, 39, 38, 37, 36, 35, 34, 33, 32, 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1})
>   
> +if false then
>   test:do_execsql_test(
>       "boundary1-2.66.ge.3",
>       "SELECT a FROM t1 WHERE rowid >= -9.22337303685477580800e+18 ORDER BY rowid",
> @@ -7610,6 +7622,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE rowid < -9.22337303685477580800e+18 ORDER BY a DESC",
>       {})
>   
> +if false then
>   test:do_execsql_test(
>       "boundary1-2.66.lt.3",
>       "SELECT a FROM t1 WHERE rowid < -9.22337303685477580800e+18 ORDER BY rowid",
> @@ -7635,6 +7649,7 @@ test:do_execsql_test(
>       "SELECT a FROM t1 WHERE rowid <= -9.22337303685477580800e+18 ORDER BY a DESC",
>       {})
>   
> +if false then
>   test:do_execsql_test(
>       "boundary1-2.66.le.3",
>       "SELECT a FROM t1 WHERE rowid <= -9.22337303685477580800e+18 ORDER BY rowid",
> @@ -7644,10 +7659,10 @@ test:do_execsql_test(
>   test:do_execsql_test(
>       "boundary1-2.66.le.5",
>       "SELECT a FROM t1 WHERE rowid <= -9.22337303685477580800e+18 ORDER BY x",
>       {})
> -

5. Stray diff.

>   test:finish_test()> diff --git a/test/sql-tap/cast.test.lua b/test/sql-tap/cast.test.lua
> index e3b7b1248..9f810bfc1 100755
> --- a/test/sql-tap/cast.test.lua
> +++ b/test/sql-tap/cast.test.lua
> @@ -64,6 +64,7 @@ test:do_execsql_test(
>           -- </cast-1.4>
>       })
>   
> +if false then

6. For this and all previous and next 'if false' - the test is
either needed or not. If a test makes no sense anymore - delete it.
If it tests something real - fix the test or code.

I do not understand why this test should not be run. It throws
and error and here it is ok. You should check that the error is
correct, an error message matches an expected one.

>   test:do_execsql_test(
>       "cast-1.5",
>       [[
> @@ -405,7 +411,9 @@ test:do_execsql_test(
>           "blob"
>           -- </cast-1.38>
>       })
> +end
>   
> +if false then
>   test:do_execsql_test(
>       "cast-1.39",
>       [[

7. Below I see this: "SELECT CAST(123.456 AS integer)".
Why is it commented? This test should pass just like
before the patch. Now it looks the same as throw an
error in C on " (int) 123.456 ". It is explicit cast of
a compatible type - why is it incorrect?

> @@ -425,6 +433,7 @@ test:do_execsql_test(
>           "integer"
>           -- </cast-1.38>
>       })
> +end
>   
>   test:do_execsql_test(
>       "cast-1.41",

I will not review other 'if false' comments. Please,
check them all by yourself and either uncomment, or delete,
or fix.

> diff --git a/test/sql-tap/collation.test.lua b/test/sql-tap/collation.test.lua
> index eb4f43a90..e8f09fdae 100755
> --- a/test/sql-tap/collation.test.lua
> +++ b/test/sql-tap/collation.test.lua
> @@ -249,9 +249,4 @@ 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"})

8. It is not a test on types. It is a test on not existing collation.
Please, fix the 'a' column type so as to fix the test.

> -
>   test:finish_test()
> diff --git a/test/sql-tap/e_select1.test.lua b/test/sql-tap/e_select1.test.lua
> index b1e113cc9..109223611 100755
> --- a/test/sql-tap/e_select1.test.lua
> +++ b/test/sql-tap/e_select1.test.lua
> @@ -332,7 +332,7 @@ test:do_execsql_test(
>   
>           -- x1: 3 rows, 2 columns
>           INSERT INTO x1 VALUES(1,'24', 'converging');
> -        INSERT INTO x1 VALUES(2, NULL, CAST(X'CB71' as TEXT));
> +        INSERT INTO x1 VALUES(2, NULL, 'Pq');\

9. Why? Explicit cast to text should work.

>           INSERT INTO x1 VALUES(3,'blonds', 'proprietary');
>   
>           -- x2: 2 rows, 3 columns> diff --git a/test/sql-tap/like3.test.lua b/test/sql-tap/like3.test.lua
> index ea6824ba7..7b42717ee 100755
> --- a/test/sql-tap/like3.test.lua
> +++ b/test/sql-tap/like3.test.lua
> @@ -67,72 +67,10 @@ test:do_execsql_test(
>           -- </like3-1.2>
>       })
>   
> -test:do_execsql_test(
> -    "like3-2.0",
> -    [[
> -        CREATE TABLE t2(a INT PRIMARY KEY, b TEXT);
> -        INSERT INTO t2 SELECT a, b FROM t1;
> -        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>
> -    })

10. Why? t2 consists of (int, text), t1 also does. So I
must be able to do "INSERT INTO t2 SELECT a, b FROM t1",
it is not? The same about other removed cases.

> -
> -test:do_execsql_test(
> -    "like3-2.1",
> -    [[
> -        SELECT a, b FROM t2 WHERE +b GLOB 'ab*' ORDER BY +a;
> -    ]], {
> -        -- <like3-2.1>
> -        1, "abc", 4, "abc"
> -        -- </like3-2.1>
> -    })
> -
> -test:do_execsql_test(
> -    "like3-2.2",
> -    [[
> -        SELECT a, b FROM t2 WHERE b>=x'6162' AND b GLOB 'ab*'
> -    ]], {
> -        -- <like3-2.2>
> -        4, "abc"
> -        -- </like3-2.2>
> -    })
> -
> -test:do_execsql_test(
> -    "like3-2.3",
> -    [[
> -        SELECT a, b FROM t2 WHERE +b>=x'6162' AND +b GLOB 'ab*'
> -    ]], {
> -        -- <like3-2.3>
> -        4, "abc"
> -        -- </like3-2.3>
> -    })
> -
> -test:do_execsql_test(
> -    "like3-2.4",
> -    [[
> -        SELECT a, b FROM t2 WHERE b GLOB 'ab*' AND b>=x'6162'
> -    ]], {
> -        -- <like3-2.4>
> -        4, "abc"
> -        -- </like3-2.4>
> -    })
> -
> -test:do_execsql_test(
> -    "like3-2.5",
> -    [[
> -        SELECT a, b FROM t2 WHERE +b GLOB 'ab*' AND +b>=x'6162'
> -    ]], {
> -        -- <like3-2.5>
> -        4, "abc"
> -        -- </like3-2.5>
> -    })
>   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;

11. Why I can not cast string to blob and vice versa?

>   ]])
> > diff --git a/test/sql-tap/selectA.test.lua b/test/sql-tap/selectA.test.lua
> index 9161cba91..6540bf166 100755
> --- a/test/sql-tap/selectA.test.lua
> +++ b/test/sql-tap/selectA.test.lua

12. I do not understand changes in this file. Neither why
is it disabled and modified at the same time. It is not too
big - 2536 lines. Please, make it work.

> diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua
> index b2d8e0589..5ecd07e87 100644
> --- a/test/sql/on-conflict.test.lua
> +++ b/test/sql/on-conflict.test.lua
> @@ -1,5 +1,9 @@
>   test_run = require('test_run').new()
> +---
> +...
>   engine = test_run:get_cfg('engine')
> +---
> +...

13. wtf ???

>   box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
>   --
>   -- Check that original SQLite ON CONFLICT clause is really


14. I do not see tests for issues 3018, 3104, 2494 fixed in
the previous commit.

My review fixes:

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

commit cf7edb9d2c3fb4aa0c2b93dd8973bdfff5d6d6e3
Author: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Date:   Thu Sep 27 19:45:17 2018 +0300

     Review fixes

diff --git a/src/box/field_def.c b/src/box/field_def.c
index 8dbead63f..3a9ff3703 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -33,6 +33,24 @@
  #include "trivia/util.h"
  #include "key_def.h"
  
+static const char *affinity_type_strs[] = {
+	/* [UNDEFINED] */ "undefined",
+	/* [BLOB - 'A'] */ "blob",
+	/* [TEXT - 'A'] */ "text",
+	/* [NUMERIC - 'A'] */ "numeric",
+	/* [INTEGER - 'A'] */ "integer",
+	/* [REAL - 'A'] */ "real",
+};
+
+const char *
+affinity_type_str(enum affinity_type type)
+{
+	if (type < 'A')
+		return affinity_type_strs[type];
+	else
+		return affinity_type_strs[type - 'A' + 1];
+}
+
  const char *field_type_strs[] = {
  	/* [FIELD_TYPE_ANY]      = */ "any",
  	/* [FIELD_TYPE_UNSIGNED] = */ "unsigned",
diff --git a/src/box/field_def.h b/src/box/field_def.h
index 05f80d409..de0ecec51 100644
--- a/src/box/field_def.h
+++ b/src/box/field_def.h
@@ -78,6 +78,11 @@ enum affinity_type {
      AFFINITY_INTEGER = 'D',
      AFFINITY_REAL = 'E',
  };
+
+/** String name of @a type. */
+const char *
+affinity_type_str(enum affinity_type type);
+
  /** \endcond public */
  
  extern const char *field_type_strs[];
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index b1d48ff72..6f5da97f1 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -326,24 +326,24 @@ applyNumericAffinity(Mem *pRec, int bTryForInt)
   * @param affinity The affinity to be applied.
   */
  static int
-mem_apply_affinity(struct Mem *record, char affinity)
+mem_apply_affinity(struct Mem *record, enum affinity_type affinity)
  {
  	if ((record->flags & MEM_Null) != 0)
  		return SQLITE_OK;
-
-	if (affinity == AFFINITY_INTEGER) {
+	switch (affinity) {
+	case AFFINITY_INTEGER:
  		if ((record->flags & MEM_Int) == MEM_Int)
  			return 0;
  		return sqlite3VdbeMemIntegerify(record, false);
-	} else if (affinity == AFFINITY_REAL) {
+	case AFFINITY_REAL:
  		if ((record->flags & MEM_Real) == MEM_Real)
  			return 0;
  		return sqlite3VdbeMemRealify(record);
-	} else if (affinity == AFFINITY_NUMERIC) {
+	case AFFINITY_NUMERIC:
  		if ((record->flags & (MEM_Real | MEM_Int)) != 0)
  			return 0;
  		return sqlite3VdbeMemNumerify(record);
-	} else if (affinity == AFFINITY_TEXT) {
+	case AFFINITY_TEXT:
  		/*
  		 * Only attempt the conversion to TEXT if there is
  		 * an integer or real representation (BLOB and
@@ -355,12 +355,13 @@ mem_apply_affinity(struct Mem *record, char affinity)
  		}
  		record->flags &= ~(MEM_Real | MEM_Int);
  		return 0;
-	} else if (affinity == AFFINITY_BLOB) {
+	case AFFINITY_BLOB:
  		if (record->flags & (MEM_Str | MEM_Blob))
  			record->flags |= MEM_Blob;
  		return 0;
+	default:
+		return -1;
  	}
-	return -1;
  }
  
  /*
@@ -1971,27 +1972,15 @@ case OP_Cast: {                  /* in1 */
  	testcase( pOp->p2==AFFINITY_REAL);
  	pIn1 = &aMem[pOp->p1];
  	rc = ExpandBlob(pIn1);
-	if (rc)
+	if (rc != 0)
  		goto abort_due_to_error;
  	rc = sqlite3VdbeMemCast(pIn1, pOp->p2);
  	UPDATE_MAX_BLOBSIZE(pIn1);
-	if (rc) {
-		const char *format;
-		if (pOp->p2 == AFFINITY_TEXT)
-			format = "Can't convert %s to TEXT";
-		else if (pOp->p2 == AFFINITY_BLOB)
-			format = "Can't convert %s to BLOB";
-		else if (pOp->p2 == AFFINITY_NUMERIC)
-			format = "Can't convert %s to NUMERIC";
-		else if (pOp->p2 == AFFINITY_INTEGER)
-			format = "Can't convert %s to INTEGER";
-		else if (pOp->p2 == AFFINITY_REAL)
-			format = "Can't convert %s to REAL";
-
-		sqlite3VdbeError(p, format, sqlite3_value_text(pIn1));
-		goto abort_due_to_error;
-	}
-	break;
+	if (rc == 0)
+		break;
+	sqlite3VdbeError(p, "Can't convert %s to %s", sqlite3_value_text(pIn1),
+			 affinity_type_str(pOp->p2));
+	goto abort_due_to_error;
  }
  #endif /* SQLITE_OMIT_CAST */
  
@@ -2394,7 +2383,7 @@ case OP_Or: {             /* same as TK_OR, in1, in2, out3 */
  	if (pIn1->flags & MEM_Null) {
  		v1 = 2;
  	} else {
-		i64 i;
+		int64_t i;
  		if ((rc = sqlite3VdbeIntValue(pIn1, &i)) != SQLITE_OK) {
  			sqlite3VdbeError(p, "Can't convert to integer %s",
  					 sqlite3_value_text(pIn1));
@@ -2406,7 +2395,7 @@ case OP_Or: {             /* same as TK_OR, in1, in2, out3 */
  	if (pIn2->flags & MEM_Null) {
  		v2 = 2;
  	} else {
-		i64 i;
+		int64_t i;
  		if ((rc = sqlite3VdbeIntValue(pIn2, &i)) != SQLITE_OK) {
  			sqlite3VdbeError(p, "Can't convert to integer %s",
  					 sqlite3_value_text(pIn2));
@@ -2443,7 +2432,7 @@ case OP_Not: {                /* same as TK_NOT, in1, out2 */
  	pOut = &aMem[pOp->p2];
  	sqlite3VdbeMemSetNull(pOut);
  	if ((pIn1->flags & MEM_Null)==0) {
-		i64 i;
+		int64_t i;
  		if ((rc = sqlite3VdbeIntValue(pIn1, &i)) != SQLITE_OK) {
  			sqlite3VdbeError(p, "Can't convert to integer %s",
  					 sqlite3_value_text(pIn1));
@@ -2467,7 +2456,7 @@ case OP_BitNot: {             /* same as TK_BITNOT, in1, out2 */
  	pOut = &aMem[pOp->p2];
  	sqlite3VdbeMemSetNull(pOut);
  	if ((pIn1->flags & MEM_Null)==0) {
-		i64 i;
+		int64_t i;
  		if ((rc = sqlite3VdbeIntValue(pIn1, &i)) != SQLITE_OK) {
  			sqlite3VdbeError(p, "Can't convert to integer %s",
  					 sqlite3_value_text(pIn1));
@@ -2780,19 +2769,9 @@ case OP_Affinity: {
  		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
  		assert(memIsValid(pIn1));
  		if ((rc = mem_apply_affinity(pIn1, cAff)) != SQLITE_OK) {
-			const char *format;
-			if (cAff == AFFINITY_TEXT)
-				format = "Can't convert %s to TEXT";
-			else if (cAff == AFFINITY_BLOB)
-				format = "Can't convert %s to BLOB";
-			else if (cAff == AFFINITY_NUMERIC)
-				format = "Can't convert %s to NUMERIC";
-			else if (cAff == AFFINITY_INTEGER)
-				format = "Can't convert %s to INTEGER";
-			else if (cAff == AFFINITY_REAL)
-				format = "Can't convert %s to REAL";
-
-			sqlite3VdbeError(p, format, sqlite3_value_text(pIn1));
+			sqlite3VdbeError(p, "Can't convert %s to %s",
+					 sqlite3_value_text(pIn1),
+					 affinity_type_str(cAff));
  			goto abort_due_to_error;
  		}
  		pIn1++;
@@ -3501,7 +3480,7 @@ case OP_SeekGT: {       /* jump, in3 */
  		if ((pIn3->flags & (MEM_Int|MEM_Real|MEM_Str))==MEM_Str) {
  			applyNumericAffinity(pIn3, 0);
  		}
-		i64 i;
+		int64_t i;
  		if ((rc = sqlite3VdbeIntValue(pIn3, &i)) != SQLITE_OK) {
  			sqlite3VdbeError(p, "Can't convert to integer %s", sqlite3_value_text(pIn1));
  			goto abort_due_to_error;
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index ead527c27..04e60a079 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -214,7 +214,7 @@ sqlite3_value_double(sqlite3_value * pVal)
  int
  sqlite3_value_int(sqlite3_value * pVal)
  {
-	i64 i;
+	int64_t i;
  	sqlite3VdbeIntValue((Mem *) pVal, &i);
  	return (int)i;
  }
@@ -222,7 +222,7 @@ sqlite3_value_int(sqlite3_value * pVal)
  sqlite_int64
  sqlite3_value_int64(sqlite3_value * pVal)
  {
-	i64 i;
+	int64_t i;
  	sqlite3VdbeIntValue((Mem *) pVal, &i);
  	return i;
  }
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index b8ceb11a7..f512ca47b 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -418,7 +418,7 @@ sqlite3VdbeMemRelease(Mem * p)
   * return the closest available 64-bit signed integer.
   */
  static int
-doubleToInt64(double r, i64 *i)
+doubleToInt64(double r, int64_t *i)
  {
  #ifdef SQLITE_OMIT_FLOATING_POINT
  	/* When floating-point is omitted, double and int64 are the same thing */
@@ -432,8 +432,8 @@ doubleToInt64(double r, i64 *i)
  	 * So we define our own static constants here using nothing
  	 * larger than a 32-bit integer constant.
  	 */
-	static const i64 maxInt = LARGEST_INT64;
-	static const i64 minInt = SMALLEST_INT64;
+	static const int64_t maxInt = LARGEST_INT64;
+	static const int64_t minInt = SMALLEST_INT64;
  
  	if (r <= (double)minInt) {
  		*i = minInt;
@@ -442,7 +442,7 @@ doubleToInt64(double r, i64 *i)
  		*i = maxInt;
  		return -1;
  	} else {
-		*i = (i64) r;
+		*i = (int64_t) r;
  		return *i != r;
  	}
  #endif
@@ -460,7 +460,7 @@ doubleToInt64(double r, i64 *i)
   * If pMem represents a string value, its encoding might be changed.
   */
  int
-sqlite3VdbeIntValue(Mem * pMem, i64 *i)
+sqlite3VdbeIntValue(Mem * pMem, int64_t *i)
  {
  	int flags;
  	assert(EIGHT_BYTE_ALIGNMENT(pMem));
@@ -528,7 +528,7 @@ sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced)
  {
  	assert(EIGHT_BYTE_ALIGNMENT(pMem));
  
-	i64 i;
+	int64_t i;
  	if (sqlite3VdbeIntValue(pMem, &i) == 0) {
  		pMem->u.i = i;
  		MemSetTypeFlag(pMem, MEM_Int);
@@ -540,10 +540,10 @@ sqlite3VdbeMemIntegerify(Mem * pMem, bool is_forced)
  	}
  
  	double d;
-	if (sqlite3VdbeRealValue(pMem, &d) || (i64)d != d) {
+	if (sqlite3VdbeRealValue(pMem, &d) || (int64_t) d != d) {
  		return SQLITE_ERROR;
  	}
-	pMem->u.i = (i64)d;
+	pMem->u.i = (int64_t) d;
  	MemSetTypeFlag(pMem, MEM_Int);
  	return SQLITE_OK;
  }
@@ -610,7 +610,7 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff)
  	    aff != AFFINITY_TEXT)
  		return SQLITE_ERROR;
  	switch (aff) {
-	case AFFINITY_BLOB:{
+	case AFFINITY_BLOB:
  		if (pMem->flags & MEM_Blob)
  			return SQLITE_OK;
  		if (pMem->flags & MEM_Str) {
@@ -620,27 +620,21 @@ sqlite3VdbeMemCast(Mem * pMem, u8 aff)
  		if (pMem->flags & MEM_Int || pMem->flags & MEM_Real)
  			return sqlite3VdbeMemStringify(pMem, 0);
  		return SQLITE_ERROR;
-	}
-	case AFFINITY_NUMERIC:{
+	case AFFINITY_NUMERIC:
  		return sqlite3VdbeMemNumerify(pMem);
-	}
-	case AFFINITY_INTEGER:{
+	case AFFINITY_INTEGER:
  		return sqlite3VdbeMemIntegerify(pMem, true);
-	}
-	case AFFINITY_REAL:{
+	case AFFINITY_REAL:
  		return sqlite3VdbeMemRealify(pMem);
-	}
-	default:{
+	default:
  		assert(aff == AFFINITY_TEXT);
  		assert(MEM_Str == (MEM_Blob >> 3));
  		pMem->flags |= (pMem->flags & MEM_Blob) >> 3;
  		sqlite3ValueApplyAffinity(pMem, AFFINITY_TEXT);
  		assert(pMem->flags & MEM_Str || pMem->db->mallocFailed);
-		pMem->flags &=
-		    ~(MEM_Int | MEM_Real | MEM_Blob | MEM_Zero);
-	}
+		pMem->flags &= ~(MEM_Int | MEM_Real | MEM_Blob | MEM_Zero);
+		return SQLITE_OK;
  	}
-	return SQLITE_OK;
  }
  
  /*
@@ -1326,7 +1320,7 @@ valueFromExpr(sqlite3 * db,	/* The database connection */
  		    sqlite3ValueFromExpr(db, pExpr->pLeft, affinity, &pVal)
  		    && pVal != 0) {
  			if ((rc = sqlite3VdbeMemNumerify(pVal)) != SQLITE_OK)
-				goto exit;
+				return rc;
  			if (pVal->flags & MEM_Real) {
  				pVal->u.r = -pVal->u.r;
  			} else if (pVal->u.i == SMALLEST_INT64) {
@@ -1342,7 +1336,7 @@ valueFromExpr(sqlite3 * db,	/* The database connection */
  		if (pVal == 0)
  			goto no_mem;
  		if ((rc = sqlite3VdbeMemNumerify(pVal)) != SQLITE_OK)
-			goto exit;
+			return rc;
  	}
  #ifndef SQLITE_OMIT_BLOB_LITERAL
  	else if (op == TK_BLOB) {
@@ -1365,7 +1359,6 @@ valueFromExpr(sqlite3 * db,	/* The database connection */
  	}
  
  	*ppVal = pVal;
- exit:
  	return rc;
  
   no_mem:
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index 4157a61e4..3ec4af9cf 100755
--- a/test/sql-tap/autoinc.test.lua
+++ b/test/sql-tap/autoinc.test.lua
@@ -618,7 +618,7 @@ test:do_catchsql_test(
              INSERT INTO t2 VALUES('asd');
      ]], {
          -- <autoinc-10.2>
-        1, "Can't convert asd to INTEGER"
+        1, "Can't convert asd to integer"
          -- </autoinc-10.2>
      })
  
diff --git a/test/sql/persistency.result b/test/sql/persistency.result
index ed0526b43..05acdd0ae 100644
--- a/test/sql/persistency.result
+++ b/test/sql/persistency.result
@@ -67,7 +67,7 @@ box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<2")
  ...
  box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<2.001")
  ---
-- error: Can't convert 2.001 to INTEGER
+- error: Can't convert 2.001 to integer
  ...
  box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<=2")
  ---
@@ -179,7 +179,7 @@ box.sql.execute("SELECT \"name\", \"opts\" FROM \"_trigger\"");
  -- ... functional
  box.sql.execute("INSERT INTO foobar VALUES ('foobar trigger test', 8888)")
  ---
-- error: Can't convert foobar trigger test to INTEGER
+- error: Can't convert foobar trigger test to integer
  ...
  box.sql.execute("SELECT * FROM barfoo WHERE foo = 9999");
  ---
diff --git a/test/sql/transition.result b/test/sql/transition.result
index 35869c69a..be2019139 100644
--- a/test/sql/transition.result
+++ b/test/sql/transition.result
@@ -64,7 +64,7 @@ box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<2")
  ...
  box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<2.001")
  ---
-- error: Can't convert 2.001 to INTEGER
+- error: Can't convert 2.001 to integer
  ...
  box.sql.execute("SELECT bar, foo, 42, 'awesome' FROM foobar WHERE foo<=2")
  ---

  parent reply	other threads:[~2018-09-27 20:24 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-17 20:32 [tarantool-patches] [PATCH 0/6] Introduce strict typing for SQL Nikita Pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 1/6] sql: split conflict action and affinity for Expr Nikita Pettik
2018-09-19  2:16   ` [tarantool-patches] " Konstantin Osipov
2018-09-27 20:24   ` Vladislav Shpilevoy
2018-10-12 11:18     ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 2/6] sql: annotate SQL functions with return type Nikita Pettik
2018-09-27 20:23   ` [tarantool-patches] " Vladislav Shpilevoy
2018-10-12 11:18     ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 3/6] sql: pass true types of columns to Tarantool Nikita Pettik
2018-09-19  2:23   ` [tarantool-patches] " Konstantin Osipov
2018-10-12 11:19     ` n.pettik
2018-09-27 20:23   ` Vladislav Shpilevoy
2018-10-12 11:18     ` n.pettik
2018-10-17 21:45       ` Vladislav Shpilevoy
2018-10-23 23:28         ` n.pettik
2018-10-29 21:32           ` Vladislav Shpilevoy
2018-11-02  2:36             ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 4/6] sql: enforce implicit type conversions Nikita Pettik
2018-09-19  2:25   ` [tarantool-patches] " Konstantin Osipov
2018-09-27 20:24   ` Vladislav Shpilevoy [this message]
2018-10-12 11:19     ` n.pettik
2018-10-17 21:45       ` Vladislav Shpilevoy
2018-10-23 23:28         ` n.pettik
2018-10-29 21:32           ` Vladislav Shpilevoy
2018-11-02  2:36             ` n.pettik
2018-11-02 11:15               ` Vladislav Shpilevoy
2018-11-02 13:26                 ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 5/6] sql: return result-set type via IProto Nikita Pettik
2018-09-19  2:26   ` [tarantool-patches] " Konstantin Osipov
2018-09-27 20:24   ` Vladislav Shpilevoy
2018-10-12 11:19     ` n.pettik
2018-10-17 21:45       ` Vladislav Shpilevoy
2018-10-23 23:28         ` n.pettik
2018-09-17 20:32 ` [tarantool-patches] [PATCH 6/6] sql: discard numeric conversion by unary plus Nikita Pettik
2018-09-27 20:24   ` [tarantool-patches] " Vladislav Shpilevoy
2018-10-12 11:19     ` n.pettik
2018-09-27 20:24 ` [tarantool-patches] Re: [PATCH 0/6] Introduce strict typing for SQL Vladislav Shpilevoy
2018-10-12 11:18   ` n.pettik
2018-11-03  2:41 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3ba8b43c-c6d5-ff34-1fd2-c302eae1a760@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=georgy@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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