Re: [Tarantool-patches] [PATCH v4 2/6] sql: refactor create_table_def and parse

[Vladislav Shpilevoy <v.shpilevoy@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 <ALTER TABLE ADD COLUMN>.
> 
> 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 <CREATE TABLE>.
> +	 */
> +	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 <CREATE TABLE> statement to be
> +	 * created has <AUTOINCREMENT>.
> +	 */
> +	bool has_autoinc;
> +	/* Id of field with <AUTOINCREMENT>. */
> +	uint32_t autoinc_fieldno;
>  	bool initiateTTrans;	/* Initiate Tarantool transaction */
>  	/** If set - do not emit byte code at all, just parse.  */
>  	bool parse_only;
>