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 17B182ADBD for ; Wed, 27 Mar 2019 10:11:15 -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 mcRqJ9_dGITe for ; Wed, 27 Mar 2019 10:11:14 -0400 (EDT) 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 96495271F2 for ; Wed, 27 Mar 2019 10:11:14 -0400 (EDT) From: "n.pettik" Message-Id: <19303130-D659-43C1-A90B-7F996E994645@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_266DAF4B-63CB-4C82-84C3-7BBB92319652" Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing Date: Wed, 27 Mar 2019 17:11:12 +0300 In-Reply-To: <6b72e8ff-ff1c-2cc7-ced0-1811df66b3dd@tarantool.org> References: <0fcc585532a1f1200a7dfd4a8e911ecf9f2c94aa.1548265148.git.korablev@tarantool.org> <45655E85-4DBB-4AC4-8161-5B783C66C688@tarantool.org> <6e4d0aae-5dbe-f8d6-3bfe-7723ea2f3d9d@tarantool.org> <78138FFB-162A-4703-9A8F-CB88FD0570EE@tarantool.org> <4841a3f7-5b21-10ea-4f58-160a5639b7d9@tarantool.org> <89C4A9F1-CB69-4F7B-95FF-9FEF751F1BB6@tarantool.org> <611D6548-9882-4FB0-9D2A-828E2CAFF83D@tarantool.org> <573b3a69-3b68-c40d-e0b5-2d36a45964fb@tarantool.org> <39DC5EC0-E609-4DB9-B6A3-9B5824B88C96@tarantool.org> <29be7940-0421-9c09-df78-be9d3977f8d7@tarantool.org> <1B451F1D-39CE-410E-8EAE-55D2DB822C49@tarantool.org> <6b72e8ff-ff1c-2cc7-ced0-1811df66b3dd@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 --Apple-Mail=_266DAF4B-63CB-4C82-84C3-7BBB92319652 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 27 Mar 2019, at 17:03, Vladislav Shpilevoy = wrote: > On 27/03/2019 16:44, n.pettik wrote: >>=20 >>> On 27 Mar 2019, at 16:29, Vladislav Shpilevoy = wrote: >>> On 27/03/2019 16:00, n.pettik wrote: >>>>=20 >>>>> On 26 Mar 2019, at 21:06, Vladislav Shpilevoy = wrote: >>>>>=20 >>>>> Thanks for the fixes! This commit LGTM. >>>>> Lets proceed to the next patches, and start >>>>> with a rebase, which is going to be hard. >>>>=20 >>>> Ok. Then I would like to clarify some details to avoid wasting = time. >>>> In previous patch version, I used next (reworked) grammar to add >>>> FK constraints using ALTER: >>>>=20 >>>> cmd ::=3D alter_table_start alter_table_action . >>>>=20 >>>> alter_table_start ::=3D ALTER TABLE fullname(Z) . (1) >>>>=20 >>>> alter_table_action ::=3D add_constraint_def. >>>> alter_table_action ::=3D drop_constraint_def. >>>> alter_table_action ::=3D rename. >>>>=20 >>>> add_constraint_def ::=3D add_constraint_start constraint_def. >>>>=20 >>>> add_constraint_start(N) ::=3D ADD CONSTRAINT nm(Z). (2) >>>> constraint_def ::=3D foreign_key_def. >>>>=20 >>>> foreign_key_def ::=3D FOREIGN KEY LP eidlist(FA) RP REFERENCES = nm(T) >>>> eidlist_opt(TA) matcharg(M) refargs(R) = defer_subclause_opt(D). >>>>=20 >>>> Now obviously I can=E2=80=99t use it since foreign_key_def should = call >>>> create_fk_def_init() which in turn requires table name and name >>>> of constraint defined in rules (1) and (2). >>>>=20 >>>> Why I want to use grammar mentioned above: it allows to remove >>>> code duplication. Rules to parse constraints are defined three = times: >>>>=20 >>>> 1. ccons rule - that is part of column definition: =E2=80=A6, a INT = REFERENCES t1); >>>> 2. tcons rule - that is part of CREATE TABLE: =E2=80=A6, CONSTRAINT = c FOREIGN KEY =E2=80=A6); >>>> 3. ALTER TABLE statement >>>>=20 >>>> All of them use the same grammar to parse statement starting from >>>> REFERENCES keyword. The same applies to UNIQUE and CHECK >>>> constraints.=20 >>>>=20 >>>> IDK how to avoid using alter_entity_def_init() and = create_constraint_def_init() >>>> and at the same time divide constraint definition into several = stages. >>>>=20 >>>> Ofc, we can still use simple approach like: >>>>=20 >>>> cmd ::=3D ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z) FOREIGN KEY >>>> LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) = matcharg(M) >>>> refargs(R) defer_subclause_opt(D) >>>>=20 >>>> cmd ::=3D ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z) UNIQUE >>>> LP sortlist(X) RP >>>>=20 >>>> cmd ::=3D ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z) PRIMARY KEY >>>> LP sortlist(X) RP >>>>=20 >>>> cmd ::=3D ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z) CHECK =E2=80=A6= >>>>=20 >>>> cmd ::=3D ALTER TABLE fullname(Z) RENAME TO nm(N) . >>>>=20 >>>> Is this OK? >>>>=20 >>>=20 >>> Obviously, it is not. Why can't you define this? >>>=20 >>> alter_table_start(T) ::=3D ALTER TABLE fullname(T) >>> alter_add_constraint(T, N) ::=3D alter_table_start(T) ADD CONSTRAINT = nm(N). >>=20 >> Lemon can=E2=80=99t use two aliases as rule parameters at the same = time. >> Instead we can introduce *another one* local struct to hold these = names. >=20 > Yes, you can define a structure in parse.y to store these two = parameters, > and unpack it back inside the concrete rules. It means, that such a > helper struct will never be stored anywhere out of parse.y. >=20 >> Anyway my initial worry was not about duplication of ALTER TABLE = CREATE CONSTRAINT, >> but rather of constraints grammar (i.e. starting from FOREIGN = KEY=E2=80=A6). >=20 > For constraints grammar you can consult the standard. I do not = remember > how it defines FOREIGN KEY rules, if it does at all. Personally for me > it looks ok. Thank you for your feedback. I=E2=80=99m going to send rebased version = of top-most patches soon. >>> cmd ::=3D alter_add_constraint(T, N) FOREIGN KEY ... >>> cmd ::=3D alter_add_constraint(T, N) UNIQUE LP sortlist(X) RP >>> cmd ::=3D alter_add_constraint(T, N) PRIMARY KEY LP sortlist(X) RP >>> cmd ::=3D alter_add_constraint(T, N) CHECK ... >>> cmd ::=3D alter_table_start RENAME TO nm(N) . >>>=20 >>> Then inside each cmd you have both table and constraint names. --Apple-Mail=_266DAF4B-63CB-4C82-84C3-7BBB92319652 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On = 27 Mar 2019, at 17:03, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
On 27/03/2019 = 16:44, n.pettik wrote:
On= 27 Mar 2019, at 16:29, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
On = 27/03/2019 16:00, n.pettik wrote:

On 26 Mar = 2019, at 21:06, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

Thanks for the fixes! This commit LGTM.
Lets = proceed to the next patches, and start
with a rebase, = which is going to be hard.

Ok. = Then I would like to clarify some details to avoid wasting time.
In previous patch version, I used next (reworked) grammar to = add
FK constraints using ALTER:

cmd ::=3D alter_table_start alter_table_action .

alter_table_start ::=3D ALTER TABLE = fullname(Z) . (1)

alter_table_action ::=3D = add_constraint_def.
alter_table_action ::=3D = drop_constraint_def.
alter_table_action ::=3D rename.

add_constraint_def ::=3D add_constraint_start = constraint_def.

add_constraint_start(N) ::=3D= ADD CONSTRAINT nm(Z). (2)
constraint_def ::=3D = foreign_key_def.

foreign_key_def ::=3D = FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T)
          &nb= sp;          eidlist_opt= (TA) matcharg(M) refargs(R) defer_subclause_opt(D).

Now obviously I can=E2=80=99t use it since foreign_key_def = should call
create_fk_def_init() which in turn requires = table name and name
of constraint defined in rules (1) and = (2).

Why I want to use grammar mentioned = above: it allows to remove
code duplication. Rules to = parse constraints are defined three times:

1.= ccons rule - that is part of column definition: =E2=80=A6, a INT = REFERENCES t1);
2. tcons rule - that is part of CREATE = TABLE: =E2=80=A6, CONSTRAINT c FOREIGN KEY =E2=80=A6);
3. = ALTER TABLE statement

All of them use the = same grammar to parse statement starting from
REFERENCES = keyword. The same applies to UNIQUE and CHECK
constraints. 

IDK how to avoid using alter_entity_def_init() and = create_constraint_def_init()
and at the same time divide = constraint definition into several stages.

Ofc, we can still use simple approach like:

cmd ::=3D ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z) =  FOREIGN KEY
          &nb= sp;LP eidlist(FA) RP REFERENCES nm(T) eidlist_opt(TA) matcharg(M)
          &nb= sp;refargs(R) defer_subclause_opt(D)

cmd = ::=3D ALTER TABLE fullname(Z) ADD CONSTRAINT nm(Z)  UNIQUE
          &nb= sp;LP sortlist(X) RP

cmd ::=3D ALTER TABLE = fullname(Z) ADD CONSTRAINT nm(Z)  PRIMARY KEY
          &nb= sp;LP sortlist(X) RP

cmd ::=3D ALTER TABLE = fullname(Z) ADD CONSTRAINT nm(Z) CHECK =E2=80=A6

cmd ::=3D ALTER TABLE fullname(Z) RENAME TO nm(N) .

Is this OK?


Obviously, it is not. Why can't = you define this?

alter_table_start(T) ::=3D = ALTER TABLE fullname(T)
alter_add_constraint(T, N) ::=3D = alter_table_start(T) ADD CONSTRAINT nm(N).

Lemon can=E2=80=99t use two aliases as rule parameters at the = same time.
Instead we can introduce *another one* local = struct to hold these names.

Yes, you can define a structure = in parse.y to store these two parameters,
and unpack it back inside the concrete rules. It means, that = such a
helper struct = will never be stored anywhere out of parse.y.

Anyway = my initial worry was not about duplication of ALTER TABLE CREATE = CONSTRAINT,
but rather of constraints grammar (i.e. = starting from FOREIGN KEY=E2=80=A6).

For constraints grammar you can = consult the standard. I do not remember
how it defines FOREIGN KEY rules, if it does at all. = Personally for me
it looks ok.

Thank you for your feedback. I=E2=80=99m going to = send rebased version of
top-most patches soon.

cmd ::=3D = alter_add_constraint(T, N) FOREIGN KEY ...
cmd ::=3D = alter_add_constraint(T, N) UNIQUE LP sortlist(X) RP
cmd = ::=3D alter_add_constraint(T, N) PRIMARY KEY LP sortlist(X) RP
cmd ::=3D alter_add_constraint(T, N) CHECK ...
cmd ::=3D alter_table_start RENAME TO nm(N) .
Then inside each cmd you have both table and constraint = names.

= --Apple-Mail=_266DAF4B-63CB-4C82-84C3-7BBB92319652--