From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <tarantool-patches-bounce@freelists.org>
Received: from localhost (localhost [127.0.0.1])
by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 678472D27B
for <tarantool-patches@freelists.org>; Tue, 23 Oct 2018 19:28:31 -0400 (EDT)
Received: from turing.freelists.org ([127.0.0.1])
by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024)
with ESMTP id I-ph1X2aLoNt for <tarantool-patches@freelists.org>;
Tue, 23 Oct 2018 19:28:31 -0400 (EDT)
Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8E7FD2D24D
for <tarantool-patches@freelists.org>; Tue, 23 Oct 2018 19:28:30 -0400 (EDT)
Content-Type: text/plain;
charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\))
Subject: [tarantool-patches] Re: [PATCH 3/6] sql: pass true types of columns
to Tarantool
From: "n.pettik" <korablev@tarantool.org>
In-Reply-To: <a0f5ebcf-b632-ffd2-f362-2b108b59c811@tarantool.org>
Date: Wed, 24 Oct 2018 02:28:19 +0300
Content-Transfer-Encoding: quoted-printable
Message-Id: <723E1B3C-AF45-4129-9C39-B5B2B04E3B28@tarantool.org>
References: <cover.1537216077.git.korablev@tarantool.org>
<9a94105515bd4f9148f302b24230d0918cfccdf9.1537216078.git.korablev@tarantool.org>
<a9049b73-c83e-7097-9ee2-3f54691b5a8f@tarantool.org>
<DA348677-686D-486A-B7D8-6038642CB293@tarantool.org>
<a0f5ebcf-b632-ffd2-f362-2b108b59c811@tarantool.org>
Sender: tarantool-patches-bounce@freelists.org
Errors-to: tarantool-patches-bounce@freelists.org
Reply-To: tarantool-patches@freelists.org
List-help: <mailto:ecartis@freelists.org?Subject=help>
List-unsubscribe: <tarantool-patches-request@freelists.org?Subject=unsubscribe>
List-software: Ecartis version 1.0.0
List-Id: tarantool-patches <tarantool-patches.freelists.org>
List-subscribe: <tarantool-patches-request@freelists.org?Subject=subscribe>
List-owner: <mailto:>
List-post: <mailto:tarantool-patches@freelists.org>
List-archive: <http://www.freelists.org/archives/tarantool-patches>
To: tarantool-patches@freelists.org
Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
>> 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 =3D=3D=
1) it is meant by SQLite that
>> + int *pSingleIdxCol, /* Tarantool. In case (nExpr =3D=3D=
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. */
>=20
> 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=E2=80=99t 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 !=3D NULL) {
>> struct SrcList *src_list =3D pExpr->x.pSelect->pSrc;
>> assert(src_list->nSrc =3D=3D 1);
>> struct Table *tab =3D src_list->a[0].pTab;
>> assert(tab !=3D NULL);
>> - struct index *pk =3D space_index(tab->space, 0);
>=20
> 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=E2=80=99ve 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 =3D=3D=
1) it is meant by SQLite that
+ int *pSingleIdxCol /* Tarantool. In case (nExpr =3D=3D=
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 =3D pParse->nTab++; /* Cursor of the RHS table */
int mustBeUnique; /* True if RHS must be unique */
Vdbe *v =3D sqlite3GetVdbe(pParse); /* Virtual machine =
being coded */
- if (pUseIdx !=3D NULL)
- *pUseIdx =3D NULL;
=20
assert(pX->op =3D=3D TK_IN);
mustBeUnique =3D (inFlags & IN_INDEX_LOOP) !=3D 0;
@@ -2490,8 +2487,6 @@ sqlite3FindInIndex(Parse * pParse, /* =
Parsing context */
|| colUsed !=3D (MASKBIT(nExpr) - =
1));
if (colUsed =3D=3D (MASKBIT(nExpr) - 1)) =
{
/* If we reach this point, that =
means the index pIdx is usable */
- if (pUseIdx !=3D NULL)
- *pUseIdx =3D idx->def;
int iAddr =3D =
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. */
=20
pLeft =3D pExpr->pLeft;
if (sqlite3ExprCheckIN(pParse, pExpr))
@@ -3019,7 +3013,7 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and =
code generating context */
eType =3D sqlite3FindInIndex(pParse, pExpr,
IN_INDEX_MEMBERSHIP | =
IN_INDEX_NOOP_OK,
destIfFalse =3D=3D destIfNull ? 0 : =
&rRhsHasNull,
- aiMap, 0, &pUseIndex);
+ aiMap, 0);
=20
assert(pParse->nErr || nVector =3D=3D 1 || eType =3D=3D =
IN_INDEX_EPH
|| eType =3D=3D IN_INDEX_INDEX_ASC || eType =3D=3D =
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 !=3D NULL) {
+ && !pExpr->is_ephemeral) {
struct SrcList *src_list =3D pExpr->x.pSelect->pSrc;
assert(src_list->nSrc =3D=3D 1);
=20
struct Table *tab =3D src_list->a[0].pTab;
assert(tab !=3D NULL);
- struct index_def *pk =3D pUseIndex;
+ struct index *pk =3D space_index(tab->space, 0);
assert(pk !=3D NULL);
=20
- uint32_t fieldno =3D pk->key_def->parts[0].fieldno;
+ uint32_t fieldno =3D pk->def->key_def->parts[0].fieldno;
enum affinity_type affinity =3D
tab->def->fields[fieldno].affinity;
- if (pk->key_def->part_count =3D=3D 1 &&
+ if (pk->def->key_def->part_count =3D=3D 1 &&
affinity =3D=3D AFFINITY_INTEGER && (int)fieldno < =
nVector) {
int reg_pk =3D 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 *);
=20
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 =3D=3D 1) {
eType =3D
sqlite3FindInIndex(pParse, pX, =
IN_INDEX_LOOP, 0, 0,
- &iSingleIdxCol, NULL);
+ &iSingleIdxCol);
} else {
Select *pSelect =3D pX->x.pSelect;
sqlite3 *db =3D pParse->db;
@@ -566,7 +566,7 @@ codeEqualityTerm(Parse * pParse, /* The parsing =
context */
eType =3D
sqlite3FindInIndex(pParse, pX,
IN_INDEX_LOOP, 0, =
aiMap,
- 0, NULL);
+ 0);
db->dbOptFlags =3D savedDbOptFlags;
testcase(aiMap !=3D 0 && aiMap[0] !=3D =
0);
pSelect->pEList =3D 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>
})
=20
@@ -385,16 +385,22 @@ test:do_test(
-- </in3-4.1>
})
=20
+-- 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>
})
=20
+-- Ephemeral table is used since collations of indexed
+-- column (rhs) 'b' and searched value (lhs) are different.
+--
test:do_test(
"in3-4.3",
function()
>=20
>> + struct index_def *pk =3D pUseIndex;
>> assert(pk !=3D NULL);
>> - uint32_t fieldno =3D pk->def->key_def->parts[0].fieldno;
>> + uint32_t fieldno =3D pk->key_def->parts[0].fieldno;
>> enum affinity_type affinity =3D
>> tab->def->fields[fieldno].affinity;
>> - if (pk->def->key_def->part_count =3D=3D 1 &&
>> + if (pk->key_def->part_count =3D=3D 1 &&
>> affinity =3D=3D AFFINITY_INTEGER && (int)fieldno < =
nVector) {
>> int reg_pk =3D rLhs + (int)fieldno;
>=20
> 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=E2=80=99t seem to be useless. Consider example:
CREATE TABLE t1 (id INT PRIMARY KEY);
SELECT 1.23 IN t1;
Now, OP_Affinity doesn=E2=80=99t 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 =3D 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(
>=20
> 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 =E2=80=98if false=E2=80=99, 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 =3D require("sqltester")
-test:plan(1894)
+test:plan(1896)
=20
--!./tcltestrunner.lua
-- 2008 December 11
@@ -125,7 +125,7 @@ test:do_test(
-- </boundary3-1.3>
})
=20
---[[test:do_execsql_test(
+test:do_execsql_test(
"boundary3-2.1.1",
"SELECT t1.a, t1.x FROM t1, t2 WHERE t1.rowid=3D72057594037927935 =
AND t2.a=3Dt1.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=3D'00ffffffffffffff'"
,{72057594037927935LL, 17})
-]]
+
>=20
>> "boundary3-2.1.1",
>> "SELECT t1.a, t1.x FROM t1, t2 WHERE t1.rowid=3D72057594037927935=
AND t2.a=3Dt1.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(
>=20
> 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 =3D require("sqltester")
-test:plan(15)
+test:plan(16)
=20
--!./tcltestrunner.lua
-- 2005 August 18
@@ -135,26 +135,26 @@ test:do_execsql_test(
-- </default-3.2>
})
=20
---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>
+})
>=20
>> +-- "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>
>=20
> Review fixes:
Thx, applied.