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

[Nikita Pettik]

On 26 Sep 16:20, Mergen Imeev wrote:
> @@ -945,10 +939,17 @@ static int
>  emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
>  			      struct index_def *idx_def)
>  {
> -	struct key_part *part = &idx_def->key_def->parts[0];
> -	int fieldno = part->fieldno;
> +	uint32_t i;
>  	char *path = NULL;
> -	if (part->path != NULL) {
> +	uint32_t fieldno = pParse->create_table_def.autoinc_field_id;
> +
> +	struct key_part *part = NULL;
> +	for (i = 0; i < idx_def->key_def->part_count; ++i) {
> +		part = &idx_def->key_def->parts[i];
> +		if (part->fieldno == fieldno)
> +			break;
> +	}
> +	if (i != idx_def->key_def->part_count && part->path != NULL) {
>  		path = sqlDbStrNDup(pParse->db, part->path, part->path_len);

Don't understand what this code is supposed to do. Could you please
explain? I've removed it and all tests are passed (obviously because
there's no way to set JSON path from SQL yet).

>  		if (path == NULL)
>  			return -1;
> @@ -3219,3 +3220,46 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
>  	sqlVdbeAddOp1(v, OP_Close, cursor);
>  	return 0;
>  }
> +
> +int
> +sql_add_autoincrement(struct Parse *parse_context, uint32_t field_id)
> +{

-> fieldno, not field id.

> +	if (parse_context->create_table_def.has_autoinc) {
> +		diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table "
> +			 "must feature at most one AUTOINCREMENT field");
> +		parse_context->is_aborted = true;
> +		return -1;
> +	}
> +	parse_context->create_table_def.has_autoinc = true;
> +	parse_context->create_table_def.autoinc_field_id = field_id;
> +	return 0;
> +}
> +
> +int
> +sql_field_id_by_name(struct Parse *parse_context, struct Expr *field_name,
> +		     uint32_t *field_id)

-> fieldno, not field id.

> +{
> +	uint32_t i;
> +	struct space_def *def = parse_context->create_table_def.new_space->def;
> +	struct Expr *name = sqlExprSkipCollate(field_name);
> +	if (name->op != TK_ID) {
> +		diag_set(ClientError, ER_INDEX_DEF_UNSUPPORTED, "Expressions");
> +		parse_context->is_aborted = true;
> +		return -1;
> +	}
> +	char *name_str = region_alloc(&parse_context->region,
> +				      strlen(name->u.zToken) + 1);
> +	strcpy(name_str, name->u.zToken);
> +	for (i = 0; i < def->field_count; ++i) {
> +		if (strcmp(def->fields[i].name, name_str) == 0)
> +			break;
> +	}

Don't understand why do you have to copy smth on region etc.
Why can't explicitly compare name->u.zToken and def->fields[i].name?

> +	/*
> +	 * It is possible that i == def->field_count. It means
> +	 * that field with AUTOINCREMENT wasn't described before

-> is not declared; please strive to avoid negations, they
make sentence understanding difficult.

It may turn out that i == def->field_count. It means that
field with AUTOINNCREMENT is declared after PRIMARY KEY...

> +	 * PRIMARY KEY() keywords. The error will be thrown later
> +	 * in parser.
> +	 */
> +	*field_id = i;
> +	return 0;
> +}
> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index 643e025..62cbf74 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -225,8 +225,16 @@ create_table_end ::= . { sqlEndTable(pParse); }
>   */
>  
>  columnlist ::= columnlist COMMA tcons.
> -columnlist ::= columnlist COMMA columnname carglist.
> -columnlist ::= columnname carglist.
> +columnlist ::= columnlist COMMA columnname carglist autoinc(I). {
> +  uint32_t field_id = pParse->create_table_def.new_space->def->field_count - 1;
> +  if (I == 1 && sql_add_autoincrement(pParse, field_id) != 0)
> +    return;
> +}
> +columnlist ::= columnname carglist autoinc(I). {
> +  uint32_t field_id = pParse->create_table_def.new_space->def->field_count - 1;
> +  if (I == 1 && sql_add_autoincrement(pParse, field_id) != 0)
> +    return;
> +}
>  columnlist ::= tcons.
>  columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
>  
> + */
> +%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). {
> +  uint32_t field_id;
> +  if (I == 1 && (sql_field_id_by_name(pParse, Y.pExpr, &field_id) != 0 ||
> +                 sql_add_autoincrement(pParse, field_id) != 0))
> +    return;
> +  A = sql_expr_list_append(pParse->db, A, Y.pExpr);
> +  sqlExprListSetSortOrder(A, Z);

Again: you dont need to set any order - it is useless now.

> +}
> +sortlist_with_autoinc(A) ::= expr(Y) sortorder(Z) autoinc(I). {
> +  uint32_t field_id;
> +  if (I == 1 && (sql_field_id_by_name(pParse, Y.pExpr, &field_id) != 0 ||
> +                 sql_add_autoincrement(pParse, field_id) != 0))
> +    return;
> +  /* A-overwrites-Y. */
> +  A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
> +  sqlExprListSetSortOrder(A, Z);
> +}
> +
>  %type sortorder {int}
>  
>  sortorder(A) ::= ASC.           {A = SORT_ORDER_ASC;}
> diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
> index 557e415..37deb9e 100644
> --- a/src/box/sql/parse_def.h
> +++ b/src/box/sql/parse_def.h
> @@ -214,6 +214,8 @@ struct create_table_def {
>  	struct rlist new_check;
>  	/** True, if table to be created has AUTOINCREMENT PK. */
>  	bool has_autoinc;
> +	/** Id of field with AUTOINCREMENT. */
> +	uint32_t autoinc_field_id;

-> fieldno

>  };
>  
>  struct create_view_def {
> @@ -461,6 +463,7 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
>  			       if_not_exists);
>  	rlist_create(&table_def->new_fkey);
>  	rlist_create(&table_def->new_check);
> +	table_def->autoinc_field_id = 0;
>  }
>  
>  static inline void
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index fbb3987..4421a48 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4419,4 +4419,25 @@ void
>  vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
>  			   const char *idx_name, const char *table_name);
>  
> +/**
> + * Add AUTOINCREMENT feature for one of INTEGER or UNSIGNED fields
> + * of PRIMARY KEY. If field_name is NULL, the field is identified
> + * by its identifier (field_id). Otherwise, field_id is ignored,
> + * and the field is identified by the name of the field that is
> + * contained in field_name.
> + *
> + * @param parse_context Parsing context.
> + * @param field_name Expr that contains field name.
> + * @param field_id Field identifier.
> + *

Comment seems to be obsolete now.

> + * @retval 0 on success.
> + * @retval -1 on error.
> + */
> +int
> +sql_add_autoincrement(struct Parse *parse_context, uint32_t field_id);
> +
> +int
> +sql_field_id_by_name(struct Parse *parse_context, struct Expr *field_name,
> +		     uint32_t *fieldno);
> +
>  #endif				/* sqlINT_H */