[tarantool-patches] Re: [PATCH 4/6] sql: enforce implicit type conversions
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Oct 18 00:45:24 MSK 2018
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 at 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);
More information about the Tarantool-patches
mailing list