[tarantool-patches] Re: [PATCH 2/6] sql: rework ALTER TABLE grammar

["n.pettik" <korablev@tarantool.org>]

> On 17 Jan 2019, at 14:51, Konstantin Osipov <kostja@tarantool.org> wrote:
> 
> * Nikita Pettik <korablev@tarantool.org> [19/01/09 15:17]:
>> +alter_table_start ::= ALTER TABLE fullname(Z) . {
>> +  pParse->constraint->table_name = Z;
>> +  pParse->constraint->name.n = 0;
>> +}
> 
> It's bikeshed at this point, but in future you will have other
> ALTER TABLE clauses: add/drop column, add/drop index,

Indexes out of scope: they are part of constraint routines.

> add/drop
> constraint. It's unclear why you initialize a specific parsing
> context (constraint definition) right in alter_table_start rule.
> OK for now.

We must save name of table before any other processing.
So, at the moment of parsing table’s name we don’t have
any notion about next actions yet.

We can use the way you suggested in previous letter:
introduce some hierarchical structures.
For instance ALTER can be implemented in the next way:

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index e38e0da08..f1a5e1974 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2707,6 +2707,105 @@ struct constraint_parse {
        struct ExprList *cols;
 };
 
+enum entity_type {
+       ENTITY_TYPE_TABLE = 0,
+       ENTITY_TYPE_INDEX,
+       ENTITY_TYPE_TRIGGER,
+       ENTITY_TYPE_CONSTRAINT,
+       ENTITY_TYPE_COLUMN
+};
+
+/***
+ * Hierarchy is following:
+ *
+ * Base structure is ALTER.
+ * ALTER is omitted only for CREATE TABLE.
+ *
+ * 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.
+ * Hence, it terms of grammar - it is terminal symbol.
+ *
+ * RENAME can be applied only to table, 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 {
+       enum entity_type entity_type;
+       struct SrcList *entity_name;
+};
+
+struct rename_entity_def {
+       struct alter_entity_def *base;
+       struct Token new_name;
+};
+
+struct create_entity_def {
+       struct alter_entity_def *base;
+       /** Statement comes with IF NOT EXISTS clause. */
+       bool if_not_exist;
+       struct Token *name;
+};
+
+struct drop_entity_def {
+       struct alter_entity_def *base;
+       /** Statement comes with IF EXISTS clause. */
+       bool if_exist;
+       struct Token entity_name;
+       /** One of CASCADE, RESTRICT */
+       int drop_behaviour;
+};
+
+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 OF trigger. */
+       struct IdList *columns;
+       /** When clause. */
+       struct Expr *when;
+       bool if_not_exist;
+
+};
+struct create_table_def {
+       struct create_entity_def *base;
+       struct space_def *table_def;
+       struct Select *select;
+};
+
+struct create_index_def {
+       struct create_entity_def *base;
+       enum sort_order sort_order;
+       enum sql_index_type idx_type;
+       struct ExprList *col_list;
+};
+
+struct create_constraint_def {
+       struct create_entity_def *base;
+       /** One of DEFERRED, IMMEDIATE. */
+       int check_time;
+};
+
+struct create_fk_def {
+       struct create_constraint_def *base;
+       struct ExprList *child_cols;
+       struct ExprList *parent_cols;
+       struct Token *parent_name;
+       enum fkey_action action;
+};
+
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index de5429498..84597bb91 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1452,8 +1452,11 @@ cmd ::= alter_table_start alter_table_action .
  * the separate rule.
  */
 alter_table_start ::= ALTER TABLE fullname(Z) . {
-  pParse->constraint.table = Z;
-  pParse->constraint.name.n = 0;
+  pParse->alter_entity_def =
+    region_alloc(pParse->region, sizeof(struct alter_entity_def));
+  pParse->alter_entity_def->entity_type = ENTITY_TYPE_TABLE;
+  pParse->alter_entity_def->parent_name = Z;
+  pParse->alter_entity_def->entity_name.n = 0;
 }
 
 alter_table_action ::= add_constraint_def.
@@ -1461,25 +1464,44 @@ alter_table_action ::= drop_constraint_def.
 alter_table_action ::= rename.
 
 rename ::= RENAME TO nm(Z). {
-  sql_alter_table_rename(pParse, &Z);
+  struct alter_entity_rename *entity_rename =
+    region_alloc(pParse->region, sizeof(struct rename_entity_def));
+  entity_rename->base = pParse->alter_entity_def;
+  entity_rename->new_name = Z;
+  pParse->alter_entity_def = (struct alter_entity_def *) entity_rename;
+  sql_alter_table_rename(pParse);
 }
 
 add_constraint_def ::= add_constraint_start constraint_def.
 
 add_constraint_start ::= ADD CONSTRAINT nm(Z). {
-  pParse->constraint.name = Z;
+  struct create_entity_def *entity_create =
+    region_alloc(pParse->region, sizeof(struct create_entity_def));
+  entity_create->base = pParse->alter_entity_def;
+  entity_create->name = Z;
+  entity_create->if_not_exist = false;
+  pParse->alter_entity_def = (struct alter_entity_def *)entity_create;
 }
 
 constraint_def ::= foreign_key_def.
 
 foreign_key_def ::= FOREIGN KEY LP eidlist(FA) RP REFERENCES nm(T)
                     eidlist_opt(TA) refargs(R) defer_subclause_opt(D). {
-  pParse->constraint.cols = FA;
-  sql_create_foreign_key(pParse, &T, TA, D, R);
+  struct create_fk_def *fk_def =
+    region_alloc(pParse->region, sizeof(struct create_fk_def));
+  fk_def->child_cols = FA;
+  fk_def->parent_cols = TA;
+  fk_def->action = R;
+  fk_def->parent_name = T;
+  sql_create_foreign_key(pParse);
 }
 
 drop_constraint_def ::= DROP CONSTRAINT nm(Z). {
-  sql_drop_foreign_key(pParse, &Z);
+    struct drop_entity_rename *entity_drop =
+      region_alloc(pParse->region, sizeof(struct drop_entity_def));
+    entity_drop->base = pParse->alter_entity_def;
+    entity_drop->new_name = Z;
+  sql_drop_foreign_key(pParse);
 }

It is easy to fix other create/drop statements to use
these structures. If it is OK to you and Vlad, I can
rework whole patch.

>> +
>> +alter_table_action ::= add_constraint_def.
>> +alter_table_action ::= drop_constraint_def.
> 
> Why use the same data structure for create and drop?

For the sake of simplicity.

> When I drop a
> constraint I specify its name and (perhaps) table name, none of
> other properties.

Not quite: drop constraint clause also may include drop behaviour:
one of CASCADE or RESTRICT.
In our implementation it is always RESTRICT.

> To capture drop context one could use:
> 
> struct drop_object_def {
>    enum object_type object_type;
>    const char *object_name;
>    const char *parent_object_name;
> };
> 
> rename of an object could be captured in a similar data structure.

Rename and drop are the easiest cases.