[tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool
n.pettik
korablev at tarantool.org
Wed Oct 24 02:28:19 MSK 2018
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index af5f3e560..95704979e 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -2313,11 +2328,12 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
>> u32 inFlags, /* IN_INDEX_LOOP, _MEMBERSHIP, and/or _NOOP_OK */
>> int *prRhsHasNull, /* Register holding NULL status. See notes */
>> int *aiMap, /* Mapping from Index fields to RHS fields */
>> - int *pSingleIdxCol /* Tarantool. In case (nExpr == 1) it is meant by SQLite that
>> + int *pSingleIdxCol, /* Tarantool. In case (nExpr == 1) it is meant by SQLite that
>> column of interest is always 0, since index columns appear first
>> in index. This is not the case for Tarantool, where index columns
>> don't change order of appearance.
>> So, use this field to store single column index. */
>> + struct index_def **pUseIdx /* Index to use. */
>
> 1. Can you now remove pSingleIdxCol? Is it just (*pUseIdx)->def->parts[0].fieldno?
Even if so, after removing additional argument containing index passed to
this function (pUseIdx, see comment below), I don’t see few-lines workaround
which may fit into this patch to handle this case.
> I've failed to find is it true or not. Please, elaborate?
This variable was added on purpose (dd5ac1528f1e5eca5786c6a6f463234030514a33)
for Tarantool and it is absent in original SQLite. As commit message says:
'This is temporary solution, all data can be stored in `aiMap` argument.'
I can suggest refactoring to use aiMap in both cases and remove that additional
argument, but does it really belong to current patch?
>> )
>> {
>> Select *p; /* SELECT to the right of IN operator */
>> @@ -3118,19 +3140,19 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */
>> sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff,
>> nVector);
>> if ((pExpr->flags & EP_xIsSelect)
>> - && !pExpr->is_ephemeral) {
>> + && !pExpr->is_ephemeral && pUseIndex != NULL) {
>> struct SrcList *src_list = pExpr->x.pSelect->pSrc;
>> assert(src_list->nSrc == 1);
>> struct Table *tab = src_list->a[0].pTab;
>> assert(tab != NULL);
>> - struct index *pk = space_index(tab->space, 0);
>
> 2. Why do you need pUseIndex here and in sqlite3FindInIndex? Why
> this old space_index(tab->space, 0) does not work?
Actually, it works fine now. Probably, it was required in some intermediate
patch state. Moreover, I found that due to this change sql-tap/in3.test.lua
contained wrong results. I’ve discarded all changes connected with this
auxiliary index and fixed test (now test looks like on origin/2.0 branch, i.e.
without changes):
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index cc4dc15a3..b9f017fca 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2330,12 +2330,11 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
u32 inFlags, /* IN_INDEX_LOOP, _MEMBERSHIP, and/or _NOOP_OK */
int *prRhsHasNull, /* Register holding NULL status. See notes */
int *aiMap, /* Mapping from Index fields to RHS fields */
- int *pSingleIdxCol, /* Tarantool. In case (nExpr == 1) it is meant by SQLite that
+ int *pSingleIdxCol /* Tarantool. In case (nExpr == 1) it is meant by SQLite that
column of interest is always 0, since index columns appear first
in index. This is not the case for Tarantool, where index columns
don't change order of appearance.
So, use this field to store single column index. */
- struct index_def **pUseIdx /* Index to use. */
)
{
Select *p; /* SELECT to the right of IN operator */
@@ -2343,8 +2342,6 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
int iTab = pParse->nTab++; /* Cursor of the RHS table */
int mustBeUnique; /* True if RHS must be unique */
Vdbe *v = sqlite3GetVdbe(pParse); /* Virtual machine being coded */
- if (pUseIdx != NULL)
- *pUseIdx = NULL;
assert(pX->op == TK_IN);
mustBeUnique = (inFlags & IN_INDEX_LOOP) != 0;
@@ -2490,8 +2487,6 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
|| colUsed != (MASKBIT(nExpr) - 1));
if (colUsed == (MASKBIT(nExpr) - 1)) {
/* If we reach this point, that means the index pIdx is usable */
- if (pUseIdx != NULL)
- *pUseIdx = idx->def;
int iAddr = sqlite3VdbeAddOp0(v, OP_Once);
VdbeCoverage(v);
sqlite3VdbeAddOp4(v, OP_Explain,
@@ -2994,7 +2989,6 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */
int addrTruthOp; /* Address of opcode that determines the IN is true */
int destNotNull; /* Jump here if a comparison is not true in step 6 */
int addrTop; /* Top of the step-6 loop */
- struct index_def *pUseIndex; /* Index to use. */
pLeft = pExpr->pLeft;
if (sqlite3ExprCheckIN(pParse, pExpr))
@@ -3019,7 +3013,7 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */
eType = sqlite3FindInIndex(pParse, pExpr,
IN_INDEX_MEMBERSHIP | IN_INDEX_NOOP_OK,
destIfFalse == destIfNull ? 0 : &rRhsHasNull,
- aiMap, 0, &pUseIndex);
+ aiMap, 0);
assert(pParse->nErr || nVector == 1 || eType == IN_INDEX_EPH
|| eType == IN_INDEX_INDEX_ASC || eType == IN_INDEX_INDEX_DESC);
@@ -3142,19 +3136,19 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */
sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff,
nVector);
if ((pExpr->flags & EP_xIsSelect)
- && !pExpr->is_ephemeral && pUseIndex != NULL) {
+ && !pExpr->is_ephemeral) {
struct SrcList *src_list = pExpr->x.pSelect->pSrc;
assert(src_list->nSrc == 1);
struct Table *tab = src_list->a[0].pTab;
assert(tab != NULL);
- struct index_def *pk = pUseIndex;
+ struct index *pk = space_index(tab->space, 0);
assert(pk != NULL);
- uint32_t fieldno = pk->key_def->parts[0].fieldno;
+ uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
enum affinity_type affinity =
tab->def->fields[fieldno].affinity;
- if (pk->key_def->part_count == 1 &&
+ if (pk->def->key_def->part_count == 1 &&
affinity == AFFINITY_INTEGER && (int)fieldno < nVector) {
int reg_pk = rLhs + (int)fieldno;
sqlite3VdbeAddOp2(v, OP_MustBeInt, reg_pk, destIfFalse);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index c79ba2acf..5af029d72 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4748,8 +4748,7 @@ void sqlite3EndBenignMalloc(void);
#define IN_INDEX_NOOP_OK 0x0001 /* OK to return IN_INDEX_NOOP */
#define IN_INDEX_MEMBERSHIP 0x0002 /* IN operator used for membership test */
#define IN_INDEX_LOOP 0x0004 /* IN operator used as a loop */
-int sqlite3FindInIndex(Parse *, Expr *, u32, int *, int *, int *,
- struct index_def **);
+int sqlite3FindInIndex(Parse *, Expr *, u32, int *, int *, int *);
void sqlite3ExprSetHeightAndFlags(Parse * pParse, Expr * p);
#if SQLITE_MAX_EXPR_DEPTH>0
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 0f26efe64..02a6d3907 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -498,7 +498,7 @@ codeEqualityTerm(Parse * pParse, /* The parsing context */
|| pX->x.pSelect->pEList->nExpr == 1) {
eType =
sqlite3FindInIndex(pParse, pX, IN_INDEX_LOOP, 0, 0,
- &iSingleIdxCol, NULL);
+ &iSingleIdxCol);
} else {
Select *pSelect = pX->x.pSelect;
sqlite3 *db = pParse->db;
@@ -566,7 +566,7 @@ codeEqualityTerm(Parse * pParse, /* The parsing context */
eType =
sqlite3FindInIndex(pParse, pX,
IN_INDEX_LOOP, 0, aiMap,
- 0, NULL);
+ 0);
db->dbOptFlags = savedDbOptFlags;
testcase(aiMap != 0 && aiMap[0] != 0);
pSelect->pEList = pOrigRhs;
diff --git a/test/sql-tap/in3.test.lua b/test/sql-tap/in3.test.lua
index 78f2ba4f0..b93cc45d0 100755
--- a/test/sql-tap/in3.test.lua
+++ b/test/sql-tap/in3.test.lua
@@ -298,7 +298,7 @@ test:do_test(
return exec_neph(" SELECT x IN (SELECT a FROM t1) FROM t2 ")
end, {
-- <in3-3.2>
- 0, 1
+ 0, 0
-- </in3-3.2>
})
@@ -385,16 +385,22 @@ test:do_test(
-- </in3-4.1>
})
+-- No need in ephemeral table since index t3_i can be used:
+-- types are matching, column 'b' is leftmost in index.
+--
test:do_test(
"in3-4.2",
function()
return exec_neph(" SELECT 'text' IN (SELECT b FROM t3)")
end, {
-- <in3-4.2>
- 0, 1
+ 0, 0
-- </in3-4.2>
})
+-- Ephemeral table is used since collations of indexed
+-- column (rhs) 'b' and searched value (lhs) are different.
+--
test:do_test(
"in3-4.3",
function()
>
>> + struct index_def *pk = pUseIndex;
>> assert(pk != NULL);
>> - uint32_t fieldno = pk->def->key_def->parts[0].fieldno;
>> + uint32_t fieldno = pk->key_def->parts[0].fieldno;
>> enum affinity_type affinity =
>> tab->def->fields[fieldno].affinity;
>> - if (pk->def->key_def->part_count == 1 &&
>> + if (pk->key_def->part_count == 1 &&
>> affinity == AFFINITY_INTEGER && (int)fieldno < nVector) {
>> int reg_pk = rLhs + (int)fieldno;
>
> 3. Why do we still need this MustBeInt here? As I understand, here we
> are trying to use an index for binary search of an LHS, but under the
> hood it is just tarantool iterators starting from certain keys, it is
> not? If it is, then tarantool iterators does key validation for us. Can
> you please check if this opcode is redundant?
It doesn’t seem to be useless. Consider example:
CREATE TABLE t1 (id INT PRIMARY KEY);
SELECT 1.23 IN t1;
Now, OP_Affinity doesn’t cast REAL to INT - I described reason
in previous review. So, without OP_MustBeInt query above would
result in
Execution failed: Supplied key type of part 0 does not match index part type: expected integer
Since 1.23 is passed to iterator over integer field. But I guess query above
is fine - it should simply return 0, not an error since queries like
SELECT * FROM t1 WHERE id = 1.123;
SELECT * FROM t1 WHERE id > 1.123;
INSERT INTO t1 VALUES (1.123);
are allowed as well.
>> diff --git a/test/sql-tap/boundary3.test.lua b/test/sql-tap/boundary3.test.lua
>> index 5b63e0539..c5d605b65 100755
>> --- a/test/sql-tap/boundary3.test.lua
>> +++ b/test/sql-tap/boundary3.test.lua
>> @@ -125,7 +125,7 @@ test:do_test(
>> -- </boundary3-1.3>
>> })
>> -test:do_execsql_test(
>> +--[[test:do_execsql_test(
>
> 4. Do not silently comment tests. We should either delete it, or fix, or
> open an issue to fix it later.
These tests are working. In previous review you asked to enable tests which
are under ‘if false’, so I forgot about simply commented tests.
diff --git a/test/sql-tap/boundary3.test.lua b/test/sql-tap/boundary3.test.lua
index c5d605b65..934d3722a 100755
--- a/test/sql-tap/boundary3.test.lua
+++ b/test/sql-tap/boundary3.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(1894)
+test:plan(1896)
--!./tcltestrunner.lua
-- 2008 December 11
@@ -125,7 +125,7 @@ test:do_test(
-- </boundary3-1.3>
})
---[[test:do_execsql_test(
+test:do_execsql_test(
"boundary3-2.1.1",
"SELECT t1.a, t1.x FROM t1, t2 WHERE t1.rowid=72057594037927935 AND t2.a=t1.a"
,{17, "00ffffffffffffff"})
@@ -134,7 +134,7 @@ test:do_execsql_test(
"boundary3-2.1.2",
"SELECT t2.* FROM t1 JOIN t2 USING(a) WHERE x='00ffffffffffffff'"
,{72057594037927935LL, 17})
-]]
+
>
>> "boundary3-2.1.1",
>> "SELECT t1.a, t1.x FROM t1, t2 WHERE t1.rowid=72057594037927935 AND t2.a=t1.a"
>> ,{17, "00ffffffffffffff"})
>> diff --git a/test/sql-tap/default.test.lua b/test/sql-tap/default.test.lua
>> index 88bfa219f..40e60d017 100755
>> --- a/test/sql-tap/default.test.lua
>> +++ b/test/sql-tap/default.test.lua
>> @@ -135,26 +135,26 @@ test:do_execsql_test(
>> -- </default-3.2>
>> })
>> -test:do_execsql_test(
>> - "default-3.3",
>> - [[
>> - CREATE TABLE t300(
>> - pk INTEGER PRIMARY KEY AUTOINCREMENT,
>> - a INT DEFAULT 2147483647,
>> - b INT DEFAULT 2147483648,
>> - c INT DEFAULT +9223372036854775807,
>> - d INT DEFAULT -2147483647,
>> - e INT DEFAULT -2147483648,
>> - f INT DEFAULT (-9223372036854775808),
>> - h INT DEFAULT (-(-9223372036854775807))
>> - );
>> - INSERT INTO t300 DEFAULT VALUES;
>> - SELECT a, b, c, d, e, f, h FROM t300;
>> - ]], {
>> - -- <default-3.3>
>> - 2147483647, 2147483648, 9223372036854775807LL, -2147483647, -2147483648, -9223372036854775808LL, 9223372036854775807LL
>> - -- </default-3.3>
>> -})
>> +--test:do_execsql_test(
>
> 5. The same.
This one works as well, simply uncommented:
diff --git a/test/sql-tap/default.test.lua b/test/sql-tap/default.test.lua
index 40e60d017..bf7594809 100755
--- a/test/sql-tap/default.test.lua
+++ b/test/sql-tap/default.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(15)
+test:plan(16)
--!./tcltestrunner.lua
-- 2005 August 18
@@ -135,26 +135,26 @@ test:do_execsql_test(
-- </default-3.2>
})
---test:do_execsql_test(
--- "default-3.3",
--- [[
--- CREATE TABLE t300(
--- pk INTEGER PRIMARY KEY AUTOINCREMENT,
--- a INT DEFAULT 2147483647,
--- b INT DEFAULT 2147483648,
--- c INT DEFAULT +9223372036854775807,
--- d INT DEFAULT -2147483647,
--- e INT DEFAULT -2147483648,
--- f INT DEFAULT (-9223372036854775808),
--- h INT DEFAULT (-(-9223372036854775807))
--- );
--- INSERT INTO t300 DEFAULT VALUES;
--- SELECT a, b, c, d, e, f, h FROM t300;
--- ]], {
--- -- <default-3.3>
--- 2147483647, 2147483648, 9223372036854775807LL, -2147483647, -2147483648, -9223372036854775808LL, 9223372036854775807LL
--- -- </default-3.3>
---})
+test:do_execsql_test(
+ "default-3.3",
+ [[
+ CREATE TABLE t300(
+ pk INTEGER PRIMARY KEY AUTOINCREMENT,
+ a INT DEFAULT 2147483647,
+ b INT DEFAULT 2147483648,
+ c INT DEFAULT +9223372036854775807,
+ d INT DEFAULT -2147483647,
+ e INT DEFAULT -2147483648,
+ f INT DEFAULT (-9223372036854775808),
+ h INT DEFAULT (-(-9223372036854775807))
+ );
+ INSERT INTO t300 DEFAULT VALUES;
+ SELECT a, b, c, d, e, f, h FROM t300;
+ ]], {
+ -- <default-3.3>
+ 2147483647, 2147483648, 9223372036854775807LL, -2147483647, -2147483648, -9223372036854775808LL, 9223372036854775807LL
+ -- </default-3.3>
+})
>
>> +-- "default-3.3",
>> +-- [[
>> +-- CREATE TABLE t300(
>> +-- pk INTEGER PRIMARY KEY AUTOINCREMENT,
>> +-- a INT DEFAULT 2147483647,
>> +-- b INT DEFAULT 2147483648,
>> +-- c INT DEFAULT +9223372036854775807,
>> +-- d INT DEFAULT -2147483647,
>> +-- e INT DEFAULT -2147483648,
>> +-- f INT DEFAULT (-9223372036854775808),
>> +-- h INT DEFAULT (-(-9223372036854775807))
>> +-- );
>> +-- INSERT INTO t300 DEFAULT VALUES;
>> +-- SELECT a, b, c, d, e, f, h FROM t300;
>> +-- ]], {
>> +-- -- <default-3.3>
>
> Review fixes:
Thx, applied.
More information about the Tarantool-patches
mailing list