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

[Nikita Pettik <korablev@tarantool.org>]

On 10 Nov 15:17, Roman Khabibov wrote:
> Hi! Thanks for the review.
> 
> > On Nov 6, 2020, at 01:17, Nikita Pettik <korablev@tarantool.org> 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.