[tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT

[Nikita Pettik]

On 21 Sep 10:22, Mergen Imeev wrote:
> Hi! Thank you for review! My answers, diff and bew patch
> below.
> 
> On Wed, Sep 18, 2019 at 08:22:27PM +0300, Nikita Pettik wrote:
> > On 28 Aug 14:47, Mergen Imeev wrote:
> > > 
> > > From f71cec86f8a19c60c2c71f78d25c473e7495b2e3 Mon Sep 17 00:00:00 2001
> > > From: Mergen Imeev <imeevma at gmail.com>
> > > Date: Tue, 27 Aug 2019 14:03:58 +0300
> > > Subject: [PATCH] sql: AUTOINCREMENT for multipart PK
> > > 
> > > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> > > index be3c5c3..371da79 100644
> > > --- a/src/box/sql/parse.y
> > > +++ b/src/box/sql/parse.y
> > > @@ -225,8 +225,34 @@ create_table_end ::= . { sqlEndTable(pParse); }
> > >   */
> > >  
> > >  columnlist ::= columnlist COMMA tcons.
> > > -columnlist ::= columnlist COMMA columnname carglist.
> > > -columnlist ::= columnname carglist.
> > > +columnlist ::= columnlist COMMA columnname carglist autoinc(I). {
> > > +  if (I == 1) {
> > > +    if (pParse->create_table_def.has_autoinc) {
> > 
> > Nit: just squash these two conditions in one diff:
> > 
> > if (I && pParse->create_table_def.has_autoinc) ...
> > 
> Not sure if that would be better. Moved this code to a new
> function.
> 
> > > +      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
> > > +               "at most one AUTOINCREMENT field");
> > > +      pParse->is_aborted = true;
> > > +      return;
> > > +    }
> > > +    pParse->create_table_def.has_autoinc = true;
> > > +    pParse->create_table_def.autoinc_field_id =
> > > +      pParse->create_table_def.new_space->def->field_count - 1;
> > > +    assert(pParse->create_table_def.autoinc_field_name == NULL);
> > 
> > Why not always use field_name? Why not move this logic to autoinc() rule?
> > This code duplication looks quite awful.
> > 
> This would be quite problematic, since we need to get the
> field name by its identifier, and then get the field
> identifier by its name. Moved this code to a new function.

Ok, but then you can do it vice versa: covert name to id right after
parsing. As far as I remember, constraint definitions always come
after format declaration. In other words, one can't create table this
way: CREATE TABLE (a INT, PRIMARY KEY (id), id INT);
So you can always point out field number using name.
 
> > > +%type sortlist_with_autoinc {ExprList*}
> > > +%destructor sortlist_with_autoinc {sql_expr_list_delete(pParse->db, $$);}
> > > +
> > > +sortlist_with_autoinc(A) ::= sortlist_with_autoinc(A) COMMA expr(Y) sortorder(Z) autoinc(I). {i
> > 
> > Why do you need expr? Expressions are prohibited in index definition.
> > 
> I just copied the "sortlist" rule and changed it a bit.

That's not a reason.

> Since after processing the AUTOINCREMENT keyword, it should
> work the same as the "sortlist" rule, I did not change the
> original arguments of the rule.

Ok, then fix (in a separate patch) sortlist as well. Or, what is better,
introduce new rule which would fulfill all requirenments for index
definition (both for auto-incremented and without auto-increment).