From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 678472D27B for ; 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 ; 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 ; 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" In-Reply-To: Date: Wed, 24 Oct 2018 02:28:19 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <723E1B3C-AF45-4129-9C39-B5B2B04E3B28@tarantool.org> References: <9a94105515bd4f9148f302b24230d0918cfccdf9.1537216078.git.korablev@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >> 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, { -- - 0, 1 + 0, 0 -- }) =20 @@ -385,16 +385,22 @@ test:do_test( -- }) =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, { -- - 0, 1 + 0, 0 -- }) =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( >> -- >> }) >> -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( -- }) =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( >> -- >> }) >> -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; >> - ]], { >> - -- >> - 2147483647, 2147483648, 9223372036854775807LL, -2147483647, = -2147483648, -9223372036854775808LL, 9223372036854775807LL >> - -- >> -}) >> +--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( -- }) =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; --- ]], { --- -- --- 2147483647, 2147483648, 9223372036854775807LL, -2147483647, = -2147483648, -9223372036854775808LL, 9223372036854775807LL --- -- ---}) +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; + ]], { + -- + 2147483647, 2147483648, 9223372036854775807LL, -2147483647, = -2147483648, -9223372036854775808LL, 9223372036854775807LL + -- +}) >=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; >> +-- ]], { >> +-- -- >=20 > Review fixes: Thx, applied.