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 5660A25DCB for ; Wed, 16 Jan 2019 15:06:40 -0500 (EST) 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 yMDZ5QAYQy90 for ; Wed, 16 Jan 2019 15:06:40 -0500 (EST) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 1231024AD6 for ; Wed, 16 Jan 2019 15:06:40 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar From: "n.pettik" In-Reply-To: <65ef264c-c582-3d4d-2f5c-269c75decd98@tarantool.org> Date: Wed, 16 Jan 2019 23:06:38 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <9D3AFBAC-9E9B-4CD9-8818-4125386985D0@tarantool.org> References: <0117c011c631182ddd64cff7a46e2b3e940bf03c.1547035183.git.korablev@tarantool.org> <65ef264c-c582-3d4d-2f5c-269c75decd98@tarantool.org> 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 Cc: Vladislav Shpilevoy > On 09/01/2019 15:13, Nikita Pettik wrote: >> Since ALTER TABLE ADD CONSTRAINT can be used to add various = constraint >> types (foreign key, unique etc), we should rework its grammar. >=20 > 1. But you still can not add a unique constraint via ADD CONSTRAINT = ... > The reworked grammar can be used for foreign keys only, until the last > commit. Yep, and what is your objection? Sorry, really can=E2=80=99t understand. >> As a reference for it lets use one from ANSI. >> Needed for #3097 >> --- >> src/box/sql/parse.y | 44 = +++++++++++++++++++++++++++++--------------- >> 1 file changed, 29 insertions(+), 15 deletions(-) >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >> index f4fdf58f2..874a67a9b 100644 >> --- a/src/box/sql/parse.y >> +++ b/src/box/sql/parse.y >> @@ -318,10 +318,8 @@ tcons ::=3D UNIQUE LP sortlist(X) RP. >> = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE);} >> tcons ::=3D CHECK LP expr(E) RP onconf. >> = {sql_add_check_constraint(pParse,&E);} >> -tcons ::=3D FOREIGN KEY LP eidlist(FA) RP >> - REFERENCES nm(T) eidlist_opt(TA) refargs(R) = defer_subclause_opt(D). { >> - sql_create_foreign_key(pParse, FA, &T, TA, D, R); >> -} >> +tcons ::=3D foreign_key_def. >=20 > 2. Sorry for a nit, but I do not like _def for non-struct things. Why = not > just foreign_key? For example, 'expr' rule is just 'expr', not = 'expr_def'. > The same about other new _def prefixes here and in the last commit. I took these names from ANSI grammar: ::=3D =E2=80=A6 I can expose it to foreign_key_definition, for instance. >=20 >> + >> %type defer_subclause_opt {int} >> defer_subclause_opt(A) ::=3D . {A =3D 0;} >> defer_subclause_opt(A) ::=3D defer_subclause(A). >> @@ -1444,22 +1442,38 @@ cmd ::=3D ANALYZE. = {sqlite3Analyze(pParse, 0);} >> cmd ::=3D ANALYZE nm(X). {sqlite3Analyze(pParse, &X);} >> //////////////////////// ALTER TABLE table ... = //////////////////////////////// >> -cmd ::=3D ALTER TABLE fullname(X) RENAME TO nm(Z). { >> - pParse->constraint->table_name =3D X; >> +cmd ::=3D alter_table_start alter_table_action . >=20 > 3. The same. How about 'alter_table_action' -> 'alter_table=E2=80=99? I used _action for the same reason: ::=3D ALTER TABLE (11.10 alter table statement) To be honest I like this naming, so I am going to keep it as is. > Also, as I understand, you did not inline alter_table_start here > because you need its C code be executed before alter_table_action, > right? Then how about a comment on it? Yep, I just followed the same pattern as for CREATE TABLE. Came up with poor comment: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 696400f51..de5429498 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1446,6 +1446,11 @@ cmd ::=3D ANALYZE nm(X). = {sqlite3Analyze(pParse, &X);} //////////////////////// ALTER TABLE table ... = //////////////////////////////// cmd ::=3D alter_table_start alter_table_action . =20 +/** + * We should get name of the table before processing + * any other rules. So, we've put this routine at + * the separate rule. + */ alter_table_start ::=3D ALTER TABLE fullname(Z) . { pParse->constraint.table =3D Z; pParse->constraint.name.n =3D 0; >> + >> +alter_table_start ::=3D ALTER TABLE fullname(Z) . { >> + pParse->constraint->table_name =3D Z; >> + pParse->constraint->name.n =3D 0; >> +} >> + >> +alter_table_action ::=3D add_constraint_def. >> +alter_table_action ::=3D drop_constraint_def. >> +/** RENAME is ANSI extension, so it comes as a very special case. */ >> +alter_table_action ::=3D rename. >=20 > 4. It is an extension but why is it special? It is just one of > the ways of table alteration. Because other actions starts from ADD/DROP/ALTER (which we still don=E2=80= =99t have). But ok, I=E2=80=99ve removed comment: diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 696400f51..82736e76a 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1453,7 +1453,6 @@ alter_table_start ::=3D ALTER TABLE fullname(Z) . = { =20 alter_table_action ::=3D add_constraint_def. alter_table_action ::=3D drop_constraint_def. -/** RENAME is ANSI extension, so it comes as a very special case. */ alter_table_action ::=3D rename. >=20 >> + >> +rename ::=3D RENAME TO nm(Z). { >> sql_alter_table_rename(pParse, &Z); >> } >> -cmd ::=3D ALTER TABLE fullname(X) ADD CONSTRAINT nm(Z) FOREIGN KEY >> - LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) = refargs(R) >> - defer_subclause_opt(D). { >> - pParse->constraint->table_name =3D X; >> - pParse->constraint->name =3D Z; >> - sql_create_foreign_key(pParse, FA, &T, TA, D, R); >> +add_constraint_def ::=3D add_constraint_start constraint_def. >> + >> +add_constraint_start ::=3D ADD CONSTRAINT nm(Z). { >> + pParse->constraint->name =3D Z; >> } >> -cmd ::=3D ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { >> - pParse->constraint->table_name =3D X; >> - sql_drop_foreign_key(pParse, &Z); >> +constraint_def ::=3D foreign_key_def. >> + >> +foreign_key_def ::=3D FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T) >> + eidlist_opt(TA) refargs(R) = defer_subclause_opt(D). { >> + sql_create_foreign_key(pParse, FA, &T, TA, D, R); >> +} >> + >> + >=20 > 5. Double empty-line. Fixed: @@ -1474,7 +1473,6 @@ foreign_key_def ::=3D FOREIGN KEY LP eidlist(FA) = RP REFERENCES nm(T) sql_create_foreign_key(pParse, &T, TA, D, R); } =20 - drop_constraint_def ::=3D DROP CONSTRAINT nm(Z). { sql_drop_foreign_key(pParse, &Z); }