[tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns to Tarantool
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Oct 18 00:45:31 MSK 2018
Hi! Thanks for the fixes! See my new ones on the branch in a
separate commit and 5 comments below.
> 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?
I've failed to find is it true or not. Please, elaborate?
> )
> {
> 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?
> + 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?
> 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.
> "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.
> +-- "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:
=================================================================
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 95704979e..cc4dc15a3 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -76,12 +76,14 @@ sqlite3ExprAffinity(Expr * pExpr)
if (pExpr->flags & EP_Generic)
return 0;
uint8_t op = pExpr->op;
+ struct ExprList *el;
if (op == TK_REGISTER)
op = pExpr->op2;
switch (op) {
case TK_SELECT:
assert(pExpr->flags & EP_xIsSelect);
- return sqlite3ExprAffinity(pExpr->x.pSelect->pEList->a[0].pExpr);
+ el = pExpr->x.pSelect->pEList;
+ return sqlite3ExprAffinity(el->a[0].pExpr);
case TK_CAST:
assert(!ExprHasProperty(pExpr, EP_IntValue));
return pExpr->affinity;
@@ -92,8 +94,8 @@ sqlite3ExprAffinity(Expr * pExpr)
pExpr->iColumn);
case TK_SELECT_COLUMN:
assert(pExpr->pLeft->flags & EP_xIsSelect);
- return sqlite3ExprAffinity(pExpr->pLeft->x.pSelect->pEList->
- a[pExpr->iColumn].pExpr);
+ el = pExpr->pLeft->x.pSelect->pEList;
+ return sqlite3ExprAffinity(el->a[pExpr->iColumn].pExpr);
case TK_PLUS:
case TK_MINUS:
case TK_STAR:
@@ -2341,7 +2343,7 @@ 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)
+ if (pUseIdx != NULL)
*pUseIdx = NULL;
assert(pX->op == TK_IN);
@@ -2488,7 +2490,7 @@ 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)
+ if (pUseIdx != NULL)
*pUseIdx = idx->def;
int iAddr = sqlite3VdbeAddOp0(v, OP_Once);
VdbeCoverage(v);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 82bc343e3..c79ba2acf 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1622,6 +1622,11 @@ struct sqlite3 {
#define SQLITE_MAGIC_ERROR 0xb5357930 /* An SQLITE_MISUSE error occurred */
#define SQLITE_MAGIC_ZOMBIE 0x64cffc7f /* Close with last statement close */
+/**
+ * SQL type definition. Now it is an alias to affinity, but in
+ * future it will have some attributes like number of chars in
+ * VARCHAR(<number of chars>).
+ */
struct type_def {
enum affinity_type type;
};
diff --git a/test/sql-tap/eqp.test.lua b/test/sql-tap/eqp.test.lua
index 24165759c..b52dff033 100755
--- a/test/sql-tap/eqp.test.lua
+++ b/test/sql-tap/eqp.test.lua
@@ -519,7 +519,7 @@ test:drop_all_tables()
test:do_execsql_test(
"5.1.0",
[[
- CREATE TABLE t1(idt1 INT PRIMARY KEY, a INT, b INT, ex TEXT)
+ CREATE TABLE t1(idt1 INT PRIMARY KEY, a INT, b INT, ex TEXT)
]])
test:do_eqp_test("5.1.1", "SELECT a, b FROM t1 WHERE a=1", {
diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
index d84e91359..9e67ff21b 100755
--- a/test/sql-tap/index1.test.lua
+++ b/test/sql-tap/index1.test.lua
@@ -735,7 +735,7 @@ test:do_execsql_test(
SELECT c FROM t6 ORDER BY a,b;
]], {
-- <index-14.1>
- 3, 2, 1, 5, 4
+ 3, 2, 1, 5, 4
-- </index-14.1>
})
@@ -835,7 +835,7 @@ test:do_execsql_test(
SELECT c FROM t6 WHERE a<'';
]], {
-- <index-14.11>
-
+
-- </index-14.11>
})
@@ -847,7 +847,7 @@ test:do_execsql_test(
SELECT * FROM t1;
]], {
-- <index-15.1>
-
+
-- </index-15.1>
})
@@ -1016,5 +1016,4 @@ if (0 > 0)
end
-::exe::
test:finish_test()
diff --git a/test/sql-tap/index3.test.lua b/test/sql-tap/index3.test.lua
index 9765381bf..4f950be09 100755
--- a/test/sql-tap/index3.test.lua
+++ b/test/sql-tap/index3.test.lua
@@ -56,7 +56,7 @@ test:do_execsql_test(
"index3-2.1",
[[
DROP TABLE t1;
- CREATE TABLE t1(a INT , b TEXT , c INT , d INT , e INT ,
+ CREATE TABLE t1(a INT , b TEXT , c INT , d INT , e INT ,
PRIMARY KEY(a), UNIQUE(b COLLATE "unicode_ci" DESC));
CREATE INDEX t1c ON t1(c);
CREATE INDEX t1d ON t1(d COLLATE binary ASC);
diff --git a/test/sql-tap/tkt-7bbfb7d442.test.lua b/test/sql-tap/tkt-7bbfb7d442.test.lua
index 3d8d423ed..ad8747ad4 100755
--- a/test/sql-tap/tkt-7bbfb7d442.test.lua
+++ b/test/sql-tap/tkt-7bbfb7d442.test.lua
@@ -161,7 +161,7 @@ if (1 > 0)
INSERT INTO InventoryControl(SKU, Variant, ControlDate) SELECT
- II.SKU AS SKU, II.Variant AS Variant, julianday('2011-08-30') AS ControlDate
+ II.SKU AS SKU, II.Variant AS Variant, julianday('2011-08-30') AS ControlDate
FROM InventoryItem II;
]])
diff --git a/test/sql-tap/tkt-9a8b09f8e6.test.lua b/test/sql-tap/tkt-9a8b09f8e6.test.lua
index b316fe701..4d08cd657 100755
--- a/test/sql-tap/tkt-9a8b09f8e6.test.lua
+++ b/test/sql-tap/tkt-9a8b09f8e6.test.lua
@@ -239,7 +239,7 @@ test:do_execsql_test(
SELECT x FROM t2 WHERE '1.0' IN (x);
]], {
-- <3.8>
-
+
-- </3.8>
})
@@ -309,7 +309,7 @@ test:do_execsql_test(
SELECT x FROM t3 WHERE '1' IN (x);
]], {
-- <4.7>
-
+
-- </4.7>
})
diff --git a/test/sql-tap/triggerB.test.lua b/test/sql-tap/triggerB.test.lua
index 00474a499..455c3cb3e 100755
--- a/test/sql-tap/triggerB.test.lua
+++ b/test/sql-tap/triggerB.test.lua
@@ -134,7 +134,7 @@ test:do_test(
c30 TEXT , c31 TEXT , c32 TEXT , c33 TEXT , c34 TEXT , c35 TEXT , c36 TEXT , c37 TEXT , c38 TEXT , c39 TEXT ,
c40 TEXT , c41 TEXT , c42 TEXT , c43 TEXT , c44 TEXT , c45 TEXT , c46 TEXT , c47 TEXT , c48 TEXT , c49 TEXT ,
c50 TEXT , c51 TEXT , c52 TEXT , c53 TEXT , c54 TEXT , c55 TEXT , c56 TEXT , c57 TEXT , c58 TEXT , c59 TEXT ,
- c60 TEXT , c61 TEXT , c62 TEXT , c63 TEXT , c64 TEXT , c65 TEXT
+ c60 TEXT , c61 TEXT , c62 TEXT , c63 TEXT , c64 TEXT , c65 TEXT
);
CREATE TABLE t3_changes(colnum INT PRIMARY KEY, oldval TEXT , newval TEXT );
INSERT INTO t3 VALUES(
diff --git a/test/sql-tap/update.test.lua b/test/sql-tap/update.test.lua
index a0b8cf45d..d0fc66ebb 100755
--- a/test/sql-tap/update.test.lua
+++ b/test/sql-tap/update.test.lua
@@ -889,7 +889,7 @@ test:do_execsql_test("update-10.1", [[
DROP TABLE test1;
CREATE TABLE t1(
a integer primary key,
- b INT UNIQUE,
+ b INT UNIQUE,
c INT , d INT ,
e INT , f INT ,
UNIQUE(c,d)
More information about the Tarantool-patches
mailing list