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