[tarantool-patches] Re: [PATCH v2 1/5] sql: introduce structs assembling DDL arguments during parsing

[Vladislav Shpilevoy]

Hi! Thanks for the patchset! See 5 comments below.

> What is more, we are going to introduce ALTER TABLE ADD CONSTRAINT
> UNIQUE With new hierarchy we can extend ALTER TABLE statement with ease:
> basic structures (alter -> create entity -> create constraint) are the
> same for .. FOREIGN KEY/UNIQUE, but the last one will be different.
> 
> Note that grammar for CREATE TABLE statement is not trivial and consists
> of wide range of sub-clauses (e.g. to parse foreign key or check
> constraints). Therefore, parts of space definition are assembled
> as soon as parser processes sub-rules. For this reason, current patch
> doesn't affect CREATE TABLE handling.

1. Why? I do not think, that "create table" port into parse_def.h is
impossible and follows from the facts above. 'Not trivial' does
not mean impossible.

As I see, you can create struct create_table_def, which will contain
Table *pNewTable, struct rlist new_fkey, uint32_t fkey_count,
bool is_new_table_autoinc. Just move some attributes from struct Parse
to this new structure. Is it possible? I think, it is worth doing,
because the code will look more consistent - any object change will go
through parse_def.h structures.

> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> new file mode 100644
> index 000000000..2f6d3d047
> --- /dev/null
> +++ b/src/box/sql/parse_def.h
> @@ -0,0 +1,170 @@
> +/*
> + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#include "sqliteInt.h"
> +#include "box/fkey.h"
> +
> +/**
> + * This file contains auxiliary structures and functions which
> + * are used only during parsing routine (see parse.y).
> + * Their main purpose is to assemble common parts of altered
> + * entities (such as name, or IF EXISTS clause) and pass them
> + * as a one object to further functions.
> + *
> + * Hierarchy is following:
> + *
> + * Base structure is ALTER.
> + * ALTER is omitted only for CREATE TABLE since table is filled
> + * with meta-information just-in-time of parsing:
> + * for instance, as soon as field's name and type are recognized
> + * they are added to space definition.
> + *
> + * DROP is general for all existing objects and includes
> + * name of object itself, name of parent object (table),
> + * IF EXISTS clause and may contain on-drop behaviour
> + * (CASCADE/RESTRICT, but now it is always RESTRICT).
> + * Hence, it terms of grammar - it is a terminal symbol.
> + *
> + * RENAME can be applied only to table (at least now, since it is
> + * ANSI extension), so it is also terminal symbol.
> + *
> + * CREATE in turn can be expanded to nonterminal symbol
> + * CREATE CONSTRAINT or to terminal CREATE TABLE/INDEX/TRIGGER.
> + * CREATE CONSTRAINT unfolds to FOREIGN KEY or UNIQUE/PRIMARY KEY.
> + *
> + * For instance:
> + * ALTER TABLE t ADD CONSTRAINT c FOREIGN KEY REFERENCES t2(id);
> + * ALTER *TABLE* -> CREATE ENTITY -> CREATE CONSTRAINT -> CREATE FK
> + *
> + * CREATE TRIGGER tr1 ...
> + * ALTER *TABLE* -> CREATE ENTITY -> CREATE TRIGGER
> + */
> +struct alter_entity_def {
> +	/** As a rule it is a name of table to be altered. */
> +	struct SrcList *entity_name;
> +};
> +
> +struct rename_entity_def {
> +	struct alter_entity_def *base;

2. Please, look at how we usually do inheritance in C.
We include base structure into a child as an attribute,
not as a pointer. So you can cast the child to the parent
even without touching 'base' field. Also, this change
together with comment 4 allows to remove parse_def.c
completely.

> +	struct Token new_name;
> +};
> +
> +struct create_entity_def {
> +	struct alter_entity_def *base;
> +	struct Token name;
> +	/** Statement comes with IF NOT EXISTS clause. */
> +	bool if_not_exist;
> +};
> +
> +struct drop_entity_def {
> +	struct alter_entity_def *base;
> +	/** Name of index/trigger/constraint to be dropped. */
> +	struct Token name;
> +	/** Statement comes with IF EXISTS clause. */
> +	bool if_exist;
> +};
> +
> +struct create_trigger_def {
> +	struct create_entity_def *base;
> +	/** One of TK_BEFORE, TK_AFTER, TK_INSTEAD. */
> +	int tr_tm;
> +	/** One of TK_INSERT, TK_UPDATE, TK_DELETE. */
> +	int op;
> +	/** Column list if this is an UPDATE trigger. */
> +	struct IdList *cols;
> +	/** When clause. */
> +	struct Expr *when;
> +};
> +
> +struct create_constraint_def {
> +	struct create_entity_def *base;
> +	/** One of DEFERRED, IMMEDIATE. */
> +	bool is_deferred;
> +};
> +
> +struct create_fk_def {
> +	struct create_constraint_def *base;
> +	struct ExprList *child_cols;
> +	struct Token *parent_name;
> +	struct ExprList *parent_cols;
> +	/**
> +	 * Encoded actions for MATCH, ON DELETE and
> +	 * ON UPDATE clauses.
> +	 */
> +	int actions;
> +};
> +
> +struct create_index_def {
> +	struct create_constraint_def *base;
> +	/** List of indexed columns. */
> +	struct ExprList *cols;
> +	/** One of _PRIMARY_KEY, _UNIQUE, _NON_UNIQUE. */
> +	enum sql_index_type idx_type;
> +	enum sort_order sort_order;
> +};
> +
> +
> +/**
> + * Below is a list of *_def constructors. All of them allocate
> + * memory for new object using parser's region: it simplifies
> + * things since their lifetime is restricted by parser.
> + *
> + * In case of OOM, they return NULL and set appropriate
> + * error code in parser's structure and re-raise error
> + * via diag_set().
> + */
> +struct alter_entity_def *
> +alter_entity_def_new(struct Parse *parse, struct SrcList *name);
> +
> +struct rename_entity_def *
> +rename_entity_def_new(struct Parse *parse, struct alter_entity_def *base,
> +		      struct Token new_name);
> +
> +struct create_entity_def *
> +create_entity_def_new(struct Parse *parse, struct alter_entity_def *base,
> +		      struct Token name, bool if_not_exists);
> +
> +struct drop_entity_def *
> +drop_entity_def_new(struct Parse *parse, struct alter_entity_def *base,
> +		    struct Token name, bool if_exist);
> +
> +struct create_constraint_def *
> +create_constraint_def_new(struct Parse *parse, struct create_entity_def *base,
> +			  bool is_deferred);
> +
> +struct create_fk_def *
> +create_fk_def_new(struct Parse *parse, struct create_constraint_def *base,
> +		  struct ExprList *child_cols, struct Token *parent_name,
> +		  struct ExprList *parent_cols, int actions);
> +
> +struct create_index_def *
> +create_index_def_new(struct Parse *parse, struct create_constraint_def *base,
> +		     struct ExprList *cols, enum sql_index_type idx_type,
> +		     enum sort_order sort_order);

3. Once you applied my comments 2 and 4, you can rename these functions to
*_create(), remove struct Parse * argument, and inline them into the header
as trivial ones.

> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 4110a5991..73d8a6a7c 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2781,6 +2780,12 @@ struct Parse {
>   	TriggerPrg *pTriggerPrg;	/* Linked list of coded triggers */
>   	With *pWith;		/* Current WITH clause, or NULL */
>   	With *pWithToFree;	/* Free this WITH object at the end of the parse */
> +	/**
> +	 * One of parse_def structures which are used to
> +	 * assemble and carry arguments of DDL routines
> +	 * from parse.y
> +	 */
> +	void *alter_entity_def;

4. Please, use union of all terminal defs from parse_def.h. It will
simplify many things. I see only one problem with it - cyclic dependency
sqliteInt.h <-> parse_def.h, which exists even now, but is just hidden
under void *. You can come up with your own solution or use one of my
owns:

     * there is only one structure, which makes impossible to remove
       include sqliteInt.h from parse_def.h - Token, stored by value in
       rename_entity_def, create_entity_def, drop_entity_def. All other
       values and other Tokens are stored by pointer, what makes it
       possible to announce them at the beginning of the file. So we can
       store it by pointer in these places too. Somehow;

     * replace these Token values with strict const char *str + int len.
       Honestly, I think, that it makes no sense to carry isReserved
       field inside SQL parser's logic. IsReserved is used by tokenizer
       and lemon only (only 3 usages in total over the whole source base).
       But it would be too big diff, not related to the patch.

     * move struct Token definition to parse_def.h;

     * finally move struct Parse into a separate file parser.h. Now you
       can include sqlitInt.h into parse_def.h and in parser.h without
       a cycle.

>   	/**
>   	 * Number of FK constraints declared within
>   	 * CREATE TABLE statement.
> diff --git a/test/sql-tap/index7.test.lua b/test/sql-tap/index7.test.lua
> index a8ce37f4b..ffb42403c 100755
> --- a/test/sql-tap/index7.test.lua
> +++ b/test/sql-tap/index7.test.lua
> @@ -397,6 +397,6 @@ test:do_catchsql_test(
>                   "_index"."id" = "_space"."id" AND
>                   "_space"."name"='TEST8';
>           ]],
> -        {0, {"pk_TEST8_2",0,"unique_C1_1",1}})
> +        {0, {"pk_unnamed_TEST8_2",0,"unique_C1_1",1}})

5. But you said "no functional changes" - what happened here?

>   
>   test:finish_test()
> -- 
> 2.15.1
> 
>