[tarantool-patches] Re: [patches] [PATCH] sql: remove support of partial indexes

["Alexander Turenko" <alexander.turenko@tarantool.org>]

[-- 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 --]