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 AF284469719 for ; Tue, 10 Nov 2020 15:17:58 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) From: Roman Khabibov In-Reply-To: <20201105221728.GA8188@tarantool.org> Date: Tue, 10 Nov 2020 15:17:55 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <28F38887-3AED-4D94-9019-1BBDCF176AEF@tarantool.org> References: <20201009134529.13212-1-roman.habibov@tarantool.org> <20201009134529.13212-3-roman.habibov@tarantool.org> <20201105221728.GA8188@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: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Nov 6, 2020, at 01:17, Nikita Pettik = wrote: >=20 > On 09 Oct 16:45, Roman Khabibov wrote: >> Move ck, fk constraint lists from struct create_table_def into new >> defs and autoincrement into struct Parse to make the code more >> reusable when implementing . >>=20 >> Needed for #3075 >> --- >>=20 >> +/** >> + * Emit code to create sequences, indexes, check and foreign key >> + * constraints appeared in . >> + */ >> +static void >> +vdbe_emit_create_constraints(struct Parse *parse, int reg_space_id) >=20 > I'd move this refactoring in a separate patch. Up to you. >=20 >> @@ -3306,15 +3324,15 @@ vdbe_emit_halt_with_presence_test(struct = Parse *parser, int space_id, >> int >> sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno) >> { >> - if (parse_context->create_table_def.has_autoinc) { >> + if (parse_context->has_autoinc) { >> diag_set(ClientError, ER_SQL_SYNTAX_WITH_POS, >> parse_context->line_count, = parse_context->line_pos, >> "table must feature at most one AUTOINCREMENT = field"); >> parse_context->is_aborted =3D true; >> return -1; >> } >> - parse_context->create_table_def.has_autoinc =3D true; >> - parse_context->create_table_def.autoinc_fieldno =3D fieldno; >> + parse_context->has_autoinc =3D true; >> + parse_context->autoinc_fieldno =3D fieldno; >> return 0; >> } >>=20 >> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h >> index cb0ecd2fc..21829b6f0 100644 >> --- a/src/box/sql/parse_def.h >> +++ b/src/box/sql/parse_def.h >> @@ -205,26 +205,20 @@ 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; >=20 > 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))=E2=80=9D. To make the code reusable, it is = easier to emit opcode for all constants after CREATE TABLE parsing. For the same reason, autoinc is separate. >> +}; >> + >> +struct create_checks_def { >=20 > 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. >=20 >> + /** List of ck_constraint_parse objects. */ >> + struct rlist checks; >> + /** Count of ck_constraint_parse objects. */ >> + uint32_t count; >> +}; >> +