From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CFD6B469710 for ; Tue, 17 Nov 2020 23:23:19 +0300 (MSK) Date: Tue, 17 Nov 2020 20:23:18 +0000 From: Nikita Pettik Message-ID: <20201117202318.GA19460@tarantool.org> References: <20201009134529.13212-1-roman.habibov@tarantool.org> <20201009134529.13212-3-roman.habibov@tarantool.org> <20201105221728.GA8188@tarantool.org> <28F38887-3AED-4D94-9019-1BBDCF176AEF@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <28F38887-3AED-4D94-9019-1BBDCF176AEF@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v4 2/5] sql: refactor create_table_def and parse List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org On 10 Nov 15:17, Roman Khabibov wrote: > Hi! Thanks for the review. > > > On Nov 6, 2020, at 01:17, Nikita Pettik wrote: > > > > On 09 Oct 16:45, Roman Khabibov wrote: > >> +++ b/src/box/sql/parse_def.h > >> @@ -205,26 +205,20 @@ struct create_entity_def { > >> - uint32_t check_count; > >> - /** Check constraint appeared in CREATE TABLE stmt. */ > >> - struct rlist new_check; > >> - /** True, if table to be created has AUTOINCREMENT PK. */ > >> - bool has_autoinc; > >> - /** Id of field with AUTOINCREMENT. */ > >> - uint32_t autoinc_fieldno; > > > > Did you consider adding create_column_def class? Imho it would > > fit better in parse hierarchy: it would derive from create_entity_def, > > has field_def, autoinc, ck, fk members. > Yes. We discussed this a lot with Vlad. This class is implemented in > the main, last patch of the patchset. Unfortunately, the check and > fk constraints lists had to be removed separately from this class, > because the check and fk descriptions can occur separately from the > column descriptions in CREATE TABLE, e.g. " CREATE TABLE t (a INT > PRIMARY KEY, CHECK (a > 0))”. To make the code reusable, it is easier > to emit opcode for all constants after CREATE TABLE parsing. For the What does 'constant' mean? IMHO it is not so 'reusable': anyway you have a lot of if (is_alter/is_alter_add_constr..) branches. But OK, since patch is ready, I believe this refactoring is not worth time required for that. > same reason, autoinc is separate. > > >> +}; > >> + > >> +struct create_checks_def { > > > > Why not create_ck_constraint_def? This naming would be more consistent > > with existing ck_contraint_def etc. The same for create_fk_constraint_def. > Because these are lists of constant defs. We decided to emphasize this. Do not get what's that supposed to mean. This is naming which is taken in our source code.. If you have any strong arguments from your discussion, please copy-paste them in the answer.