[tarantool-patches] Re: [patches] [PATCH] Ban of REINDEX syntax.

[Alexander Turenko]

Hi, Ivan!

Congratulations with the first contribution!

Consider the review below. Please, update the patch accordingly and send again.

WBR, Alexander Turenko.
----

Common:
- I suggest to have real name in the 'user.name' git-config setting
- rebase on top of the current 2.0

Subject line:
- add 'sql:' prefix
- remove period at the end

>Среда, 14 марта 2018, 21:12 +03:00 от vanyail < vanyail at yandex.ru >:
>
>This change cut REINDEX syntax from keywords and also disables tests which were using this syntax.

Wrap commit message lines to be 72 symbols wide.

>
>This patch is not totally removing this syntax because it will be allowed in future releases.

I would say that the change is about parser, but the implementation under hood
was hold as is, because it is planned to fix and enable this feature in the 
scope of #3195.

>
>Fixes #2174
>---

Add links to the issue and the branch here (right below --- mark).

> extra/mkkeywordhash.c                            |    6 -
> src/box/sql/keywordhash.h                        |  614 +++++-----
> src/box/sql/parse.c                              | 1413 +++++++++++-----------
> src/box/sql/parse.y                              |    7 -
> test/sql-tap/gh-2174-ban-reindex-syntax.test.lua |   15 +
> test/sql-tap/keyword1.test.lua                   |    3 +-
> test/sql-tap/suite.ini                           |    3 +-
> 7 files changed, 1018 insertions(+), 1043 deletions(-)
> create mode 100755 test/sql-tap/gh-2174-ban-reindex-syntax.test.lua


>diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
>index 914fc53..738d130 100644
>--- a/src/box/sql/parse.y
>+++ b/src/box/sql/parse.y
>@@ -1472,13 +1472,6 @@ cmd ::= DROP TRIGGER ifexists(NOERR) fullname(X). {
> }
> %endif  !SQLITE_OMIT_TRIGGER
>
>-////////////////////////// REINDEX collation //////////////////////////////////
>-%ifndef SQLITE_OMIT_REINDEX
>-cmd ::= REINDEX.                {sqlite3Reindex(pParse, 0, 0);}
>-cmd ::= REINDEX nm(X).          {sqlite3Reindex(pParse, &X, 0);}
>-cmd ::= REINDEX nm(X) ON nm(Y). {sqlite3Reindex(pParse, &X, &Y);}
>-%endif  SQLITE_OMIT_REINDEX
>-

It is better to leave the REINDEX parser code commented (see the ALTER ADD 
COLUMN case below). It is good to link the issue in the scope of which it was
commented and the issue in the scope of which it will be fixed and enabled.

>
> /////////////////////////////////// ANALYZE ///////////////////////////////////
> %ifndef SQLITE_OMIT_ANALYZE
> cmd ::= ANALYZE.                {sqlite3Analyze(pParse, 0);}
>diff --git a/test/sql-tap/gh-2174-ban-reindex-syntax.test.lua b/test/sql-tap/gh-2174-ban-reindex-syntax.test.lua
>new file mode 100755
>index 0000000..97c8390
>--- /dev/null
>+++ b/test/sql-tap/gh-2174-ban-reindex-syntax.test.lua
>@@ -0,0 +1,15 @@
>+#!/usr/bin/env tarantool

It would be food to note that the test is to be deleted in the scope of #3195.

>
>+test = require("sqltester")
>+test:plan(1)
>+
>+test:execsql("DROP TABLE IF EXISTS t1");
>+test:execsql("CREATE TABLE t1(a INT PRIMARY KEY)");
>+test:execsql("CREATE INDEX i1 on t1(a)");
>+
>+local ok = pcall(test.execsql, test, [[
>+    REINDEX i1 ON t1
>+]])
>+
>+test:ok(not ok, 'reindex syntax must be banned')
>+
>+test:finish_test()
>diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-tap/keyword1.test.lua
>index 23a561f..fbcd173 100755
>--- a/test/sql-tap/keyword1.test.lua
>+++ b/test/sql-tap/keyword1.test.lua
>@@ -1,6 +1,6 @@
> #!/usr/bin/env tarantool
> test = require("sqltester")
>-test:plan(176)
>+test:plan(175)
>
> --!./tcltestrunner.lua
> -- 2009 January 29
>@@ -107,7 +107,6 @@ local bannedkws = {
> "primary",
> "recursive",
> "references",
>-"reindex",
> "release",
> "rename",
> "replace",
>diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini
>index 0bc9e83..eac10a9 100644
>--- a/test/sql-tap/suite.ini
>+++ b/test/sql-tap/suite.ini
>@@ -1,5 +1,6 @@
> [default]
> core = app
>-description = Database tests with #! using TAP
>+description = Some test are banned until REINDEX syntax will not be allowed. Database tests with #! using TAP 

This information should not be part of the description.

>
>+disabled = reindex.test.lua gh2130-index-refer-table.test.lua misc3.test.lua 

State explicitly the reason why each test was disabled or, better, when exactly
it should be enabled. It may look like so:

disabled = \
    reindex.test.lua ; to be enabled in #3195 \
    gh2130-index-refer-table.test.lua ; to be enabled in #3195 \
    misc3.test.lua ; to be enabled in #3195

>
> lua_libs = lua/sqltester.lua ../sql/lua/sql_tokenizer.lua ../box/lua/identifier.lua
> is_parallel = True
>-- 
>2.7.4
>