[tarantool-patches] Re: [PATCH] sql: ban of REINDEX syntax

[n.pettik]

Hello. See comments below.

>This change cut

Misspelling: cut(s).

>The reason described in 'parse.y' in comment related
>to '%fallback'.

Don't reffer to comments in your commit message. Instead, provide
short desctiption/explanation.

Also put link to the issue in your cover letter.

>---
>
>branch: https://github.com/tarantool/tarantool/tree/gh-2174-ban-reindex-syntax
>---

Don't add extra '---', just put issue and branch right after
auto-generated delimiter.

>+++ b/src/box/sql/parse.y
>@@ -202,6 +202,17 @@ columnname(A) ::= nm(A) typetoken(Y). {sqlite3AddColumn(pParse,&A,&Y);}
>// fallback to ID if they will not parse as their original value.
>// This obviates the need for the "id" nonterminal.
>//
>+// A keyword is checked for being a reserve one in `nm`, before
>+// processing of this %fallback directive. Reserved keywords included
>+// here to avoid the situation when a keyword has no usages within
>+// `parse.y` file (a keyword can have more or less usages depending on
>+// compiler defines). When a keyword has no usages it is excluded
>+// from autogenerated file `parse.h` that lead to compile-time error.
>+//
>+// See [1] for more information.
>+//
>+// [1]: https://www.sqlite.org/src/info/007aec11333432e0
>+//

I would better put link to commit message or to comments of original
issue. I stick to the point that source code doesn't seem to be good
place for links. Also, in Tarantool we use C-like commenting style.

>@@ -1473,11 +1484,12 @@ 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
>+/* gh-2174: Commended until REINDEX is implemented in scope of gh-3195 */
>+/* %ifndef SQLITE_OMIT_REINDEX */
>+/* cmd ::= REINDEX.                {sqlite3Reindex(pParse, 0, 0);} */
>+/* cmd ::= REINDEX nm(X).          {sqlite3Reindex(pParse, &X, 0);} */

This variant of REINDEX syntax should be removed completely, since in
our SQL index naming exists only in scope of tables, i.e. two tables
are able to feature indexes with the same name.

>+++ b/test/sql-tap/gh-2174-ban-reindex-syntax.test.lua
>@@ -0,0 +1,17 @@
>+#!/usr/bin/env tarantool
>+
>+-- this test will be deleted in scope of #3195
>+test = require("sqltester")
>+test:plan(1)
>+
>+test:execsql("DROP TABLE IF EXISTS t1");

AFAIK in sql-tap tests you don't need to make any preliminary clean-up:
each test suite starts on brand new instance of Tarantool.

>+test:execsql("CREATE TABLE t1(a INT PRIMARY KEY)");
>+test:execsql("CREATE INDEX i1 on t1(a)");

You have banned three variants of REINDEX syntax, but in your
tests you check only one. Add more test cases.

Finally, I would suggest to use special testing facilities instead of
primitive pcall (such as test:do_catchsql_test function).
You can investigate other test examples in sql-tap.