From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 dev.tarantool.org (Postfix) with ESMTPS id E0B68469719 for ; Thu, 17 Sep 2020 17:43:54 +0300 (MSK) References: <20200911215115.6622-1-roman.habibov@tarantool.org> <20200911215115.6622-3-roman.habibov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <36bb1beb-a59a-57d9-a1ef-c0ef5af49d35@tarantool.org> Date: Thu, 17 Sep 2020 16:43:53 +0200 MIME-Version: 1.0 In-Reply-To: <20200911215115.6622-3-roman.habibov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v4 2/6] 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 , tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! On 11.09.2020 23:51, Roman Khabibov wrote: > Move ck, fk constraint lists and autoincrement info from > struct create_table_def to struct Parse 1. You moved them into new defs, not into struct Parse. You need to revisit some commit messages in this branch after review fixes are done. > to make the code more > reusable when implementing . > > Needed for #3075 > --- > diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h > index cb0ecd2fc..c44e3dbbe 100644 > --- a/src/box/sql/parse_def.h > +++ b/src/box/sql/parse_def.h > @@ -205,26 +205,16 @@ struct create_entity_def { > struct create_table_def { > struct create_entity_def base; > struct space *new_space; > - /** > - * Number of FK constraints declared within > - * CREATE TABLE statement. > - */ > - uint32_t fkey_count; > - /** > - * Foreign key constraint appeared in CREATE TABLE stmt. > - */ > - struct rlist new_fkey; > - /** > - * Number of CK constraints declared within > - * CREATE TABLE statement. > - */ > - 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; > +}; > + > +struct create_checks_def { > + struct rlist checks; > + uint32_t count; > +}; > + > +struct create_fkeys_def { > + struct rlist fkeys; > + uint32_t count; *. All struct memebers should have a comment. At least it is worth documenting what are the members of checks and fkeys lists. > }; > > struct create_view_def { > @@ -500,12 +501,12 @@ create_view_def_init(struct create_view_def *view_def, struct Token *name, > } > > static inline void > -create_table_def_destroy(struct create_table_def *table_def) > +fkeys_def_destroy(struct create_fkeys_def *fkeys_def) 2. Methods of a struct should have a prefix with the struct name. It should be create_fkeys_def_destroy(). > { > - if (table_def->new_space == NULL) > + if (fkeys_def->count == 0) > return; > struct fk_constraint_parse *fk; > - rlist_foreach_entry(fk, &table_def->new_fkey, link) > + rlist_foreach_entry(fk, &fkeys_def->fkeys, link) > sql_expr_list_delete(sql_get(), fk->selfref_cols); > } > > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 5913d7614..7057c20b6 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -2257,6 +2257,18 @@ struct Parse { > * sqlEndTable() function). > */ > struct create_table_def create_table_def; > + /* > + * FK and CK constraints appeared in a . > + */ > + struct create_fkeys_def fkeys_def; > + struct create_checks_def checks_def; 3. fkeys_def -> create_fkeys_def. checks_def -> create_checks_def. > + /* > + * True, if column within a statement to be > + * created has . > + */ > + bool has_autoinc; > + /* Id of field with . */ > + uint32_t autoinc_fieldno; > bool initiateTTrans; /* Initiate Tarantool transaction */ > /** If set - do not emit byte code at all, just parse. */ > bool parse_only; >