From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3A4652C2FC for ; Wed, 21 Mar 2018 11:37:43 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 7wWzEeoWu8Am for ; Wed, 21 Mar 2018 11:37:43 -0400 (EDT) Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 710FC2C278 for ; Wed, 21 Mar 2018 11:37:42 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: ban of REINDEX syntax From: "n.pettik" In-Reply-To: <20180321130224.7in37pr56xg6h5cr@tkn_work_nb> Date: Wed, 21 Mar 2018 18:37:38 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <4A8FE286-3EDE-460F-8AAA-6A643104B41A@tarantool.org> References: <1521120892.753756821@f173.i.mail.ru> <1521630368-2969-1-git-send-email-vanyail@yandex.ru> <20180321130224.7in37pr56xg6h5cr@tkn_work_nb> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, vanyail@yandex.ru Cc: Alexander Turenko 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) ::=3D 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 ::=3D DROP TRIGGER ifexists(NOERR) = fullname(X). { >%endif !SQLITE_OMIT_TRIGGER > >////////////////////////// REINDEX collation = ////////////////////////////////// >-%ifndef SQLITE_OMIT_REINDEX >-cmd ::=3D REINDEX. {sqlite3Reindex(pParse, 0, 0);} >-cmd ::=3D REINDEX nm(X). {sqlite3Reindex(pParse, &X, 0);} >-cmd ::=3D 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 ::=3D REINDEX. {sqlite3Reindex(pParse, 0, 0);} = */ >+/* cmd ::=3D 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 =3D 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.