* [tarantool-patches] Re: [patches] [PATCH] sql: remove support of partial indexes
[not found] <1521053795-8318-1-git-send-email-gleb-skiba@mail.ru>
@ 2018-03-15 16:16 ` Alexander Turenko
0 siblings, 0 replies; only message in thread
From: Alexander Turenko @ 2018-03-15 16:16 UTC (permalink / raw)
To: Gleb; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 7754 bytes --]
Hi Gleb!
Congratulations with the first contribution!
Please, consider the review below, update the patch and send it again.
WBR, Alexander Turenko.
----
Common:
- Add a test that makes sure tarantool gives an error at banned syntax. Make
sure you pointed out the issue after which the test should be removed
(#2626). Name it like sql-tap/gh-2165-ban-partial-index.test.lua. Use of
do_catchsql_test or just pcall (in case of problems with the first option).
- Please, comment why do you comment a test case, mention the issue in the
scope of which you do that and the issue after which it should be
uncommented.
>Среда, 14 марта 2018, 21:56 +03:00 от Gleb < gleb-skiba@mail.ru >:
>
>Issue from https://github.com/tarantool/tarantool/issues/2165 .
>Source from https://github.com/tarantool/tarantool/tree/gh-2165-remove-support-partial-indexes .
Add extra notes (that are not part of the commit) right after the --- mark (to
be ignored by `git am` and do not become part of the commit message).
>Remove support partial indexes, and comment tests on this.
>Rewrite commit in template way.
The second clause is something unrelated to the commit.
>
>Fixes #2165
>---
Here should be extra notes, because here they will be ignored by `git am`.
> src/box/sql/build.c | 11 +-
> src/box/sql/parse.c | 205 +++++++++---------
> src/box/sql/parse.y | 3 +-
> test/sql-tap/analyze9.test.lua | 144 ++++++-------
> test/sql-tap/autoindex4.test.lua | 34 +--
> test/sql-tap/fkey1.test.lua | 65 +++---
> test/sql-tap/index6.test.lua | 437 ++++++++++++++++++++-------------------
> test/sql-tap/index7.test.lua | 81 ++++----
> 8 files changed, 491 insertions(+), 489 deletions(-)
>
>diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>index 9ad0c06..e39fa87 100644
>--- a/src/box/sql/build.c
>+++ b/src/box/sql/build.c
>@@ -76,7 +76,7 @@ sqlite3FinishCoding(Parse * pParse)
> return;
> }
>
>-/* Begin by generating some termination code at the end of the
>+/* Begin by generating some termination code at the end of the
It is strongly discouraged to have trailing spaces (at the end of the line).
Consider `git diff` before a commit, it helps to avoid such mistakes.
> * vdbe program
> */
> v = sqlite3GetVdbe(pParse);
>@@ -1031,8 +1031,8 @@ sqlite3AddPrimaryKey(Parse * pParse,/* Parsing context */
> sqlite3ErrorMsg(pParse, "AUTOINCREMENT is only allowed on an "
> "INTEGER PRIMARY KEY or INT PRIMARY KEY");
> } else {
>-sqlite3CreateIndex(pParse, 0, 0, pList, onError, 0,
>- 0, sortOrder, 0, SQLITE_IDXTYPE_PRIMARYKEY);
>+sqlite3CreateIndex(pParse, 0, 0, pList, onError, 0, 0,
>+ sortOrder, 0, SQLITE_IDXTYPE_PRIMARYKEY);
> pList = 0;
> }
Here only formatting was changed, but really nothing. Please, revert.
>
>@@ -2961,12 +2961,10 @@ sqlite3CreateIndex(Parse * pParse,/* All information about this parse */
> /* Tarantool have access to each column by any index */
> pIndex->isCovering = 1;
> if (pPIWhere) {
>-sqlite3ResolveSelfReference(pParse, pTab, NC_PartIdx, pPIWhere,
>- 0);
>+sqlite3ResolveSelfReference(pParse, pTab, NC_PartIdx, pPIWhere,0);
> pIndex->pPartIdxWhere = pPIWhere;
> pPIWhere = 0;
> }
The same. Please, revert.
>-
> /* Analyze the list of expressions that form the terms of the index and
> * report any errors. In the common case where the expression is exactly
> * a table column, store that column in aiColumn[]. For general expressions,
>@@ -3207,7 +3205,6 @@ sqlite3CreateIndex(Parse * pParse,/* All information about this parse */
> exit_create_index:
> if (pIndex)
> freeIndex(db, pIndex);
>-sqlite3ExprDelete(db, pPIWhere);
> sqlite3ExprListDelete(db, pList);
> sqlite3SrcListDelete(db, pTblName);
> sqlite3DbFree(db, zName);
I think the entire build.c file should be leaved as it was before.
>diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>index 914fc53..c14622f 100644
>--- a/src/box/sql/parse.y
>+++ b/src/box/sql/parse.y
>@@ -1237,7 +1237,8 @@ paren_exprlist(A) ::= LP exprlist(X) RP. {A = X;}
> //
> cmd ::= createkw(S) uniqueflag(U) INDEX ifnotexists(NE) nm(X)
> ON nm(Y) LP sortlist(Z) RP where_opt(W). {
>- sqlite3CreateIndex(pParse, &X,
>+ assert(W == NULL);
>+ sqlite3CreateIndex(pParse, &X,
> sqlite3SrcListAppend(pParse->db,0,&Y), Z, U,
> &S, W, SQLITE_SO_ASC, NE, SQLITE_IDXTYPE_APPDEF);
> }
Release/RelWithDebInfo builds has no asserts. Debug build will be killed by
SIGABRT (this is what assert does). The change needed here is remove
where_opt(W) and replace W parameter of the sqlite3CreateIndex() call with 0.
>diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
>index 4ce575e..472bad7 100755
>--- a/test/sql-tap/analyze9.test.lua
>+++ b/test/sql-tap/analyze9.test.lua
>@@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
>-test:do_execsql_test(
>- 17.6,
>- [[
>- EXPLAIN QUERY PLAN SELECT * FROM t1 WHERE d IS NOT NULL AND a=0 AND b=0 AND c=10;
>- ]], {
>- -- <17.6>
>- 0, 0, 0, "SEARCH TABLE T1 USING COVERING INDEX I2 (C=? AND D>?)"
>- -- </17.6>
>- })
>+--test:do_execsql_test(
>+-- 17.6,
>+-- [[
>+-- EXPLAIN QUERY PLAN SELECT * FROM t1 WHERE d IS NOT NULL AND a=0 AND b=0 AND c=10;
>+-- ]], {
>+-- -- <17.6>
>+-- 0, 0, 0, "SEARCH TABLE T1 USING COVERING INDEX I2 (C=? AND D>?)"
>+-- -- </17.6>
>+-- })
>
The indent is broken on the line 1176: `-- 17.6,`.
> ---------------------------------------------------------------------------
>
>diff --git a/test/sql-tap/autoindex4.test.lua b/test/sql-tap/autoindex4.test.lua
>index 45bae48..874aad7 100755
>--- a/test/sql-tap/autoindex4.test.lua
>+++ b/test/sql-tap/autoindex4.test.lua
>@@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
>-test:plan(7)
>+test:plan(6)
>
> --!./tcltestrunner.lua
> -- 2014-10-24
>@@ -108,22 +108,22 @@ test:do_execsql_test(
> -- </autoindex4-3.0>
> })
>
>-test:do_execsql_test(
>- "autoindex4-3.1",
>- [[
>- CREATE INDEX Items_x1 ON Items(ItemName,Name) WHERE ItemName = 'dummy';
>-
>- SELECT Items.ItemName
>- FROM Items
>- LEFT JOIN A ON (A.Name = Items.ItemName and Items.ItemName = 'dummy')
>- LEFT JOIN B ON (B.Name = Items.ItemName)
>- WHERE Items.Name = 'Parent'
>- ORDER BY Items.ItemName;
>- ]], {
>- -- <autoindex4-3.1>
>- "Item1", "Item2"
>- -- </autoindex4-3.1>
>- })
>+--test:do_execsql_test(
>+-- "autoindex4-3.1",
>+-- [[
>+-- CREATE INDEX Items_x1 ON Items(ItemName,Name) WHERE ItemName = 'dummy';
>+--
>+-- SELECT Items.ItemName
>+-- FROM Items
>+-- LEFT JOIN A ON (A.Name = Items.ItemName and Items.ItemName = 'dummy')
>+-- LEFT JOIN B ON (B.Name = Items.ItemName)
>+-- WHERE Items.Name = 'Parent'
>+-- ORDER BY Items.ItemName;
>+-- ]], {
>+-- -- <autoindex4-3.1>
>+-- "Item1", "Item2"
>+-- -- </autoindex4-3.1>
>+-- })
>
> test:finish_test()
>
The indent is broken: 117: `-- FROM Items`.
>diff --git a/test/sql-tap/index6.test.lua b/test/sql-tap/index6.test.lua
>index 2aa97e8..d2edc56 100755
>--- a/test/sql-tap/index6.test.lua
>+++ b/test/sql-tap/index6.test.lua
>@@ -1,6 +1,7 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
>-test:plan(14)
>+test:plan(0)
The test can be disabled entirely in the test/sql-tap/suite.ini using the
`disabled` parameter like so:
disabled = \
sql-tap/index6.test.lua ; to be enaled after #2626
[-- Attachment #2: Type: text/html, Size: 10301 bytes --]
^ permalink raw reply [flat|nested] only message in thread