[tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT
Mergen Imeev
imeevma at tarantool.org
Wed Oct 9 17:30:59 MSK 2019
Hi! Thank you for review. My answers, diff and new patch
below.
On Mon, Sep 30, 2019 at 04:56:09PM +0300, Nikita Pettik wrote:
> 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).
>
This code was added when JSON path was implemented. I
removed it.
> > 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.
>
Fixed.
> > + 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.
>
Fixed.
> > +{
> > + 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?
>
Fixed.
> > + /*
> > + * 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...
>
Removed, replaced by error.
> > + * 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.
>
Removed sortorder from sortlist_with_autoinc rule.
> > +}
> > +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
>
Fixed.
> > };
> >
> > 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.
>
Fixed.
> > + * @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 */
Diff:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 3d642ed..edaf5d0 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -936,25 +936,9 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
}
static int
-emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
- struct index_def *idx_def)
+emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id)
{
- uint32_t i;
- char *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);
- if (path == NULL)
- return -1;
- path[part->path_len] = 0;
- }
+ uint32_t fieldno = pParse->create_table_def.autoinc_fieldno;
Vdbe *v = sqlGetVdbe(pParse);
int first_col = pParse->nMem + 1;
@@ -973,9 +957,7 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
sqlVdbeAddOp2(v, OP_Integer, fieldno, first_col + 4);
/* 5. Field path. */
- sqlVdbeAddOp4(v, OP_String8, 0, first_col + 5, 0,
- path != NULL ? path : "",
- path != NULL ? P4_DYNAMIC : P4_STATIC );
+ sqlVdbeAddOp4(v, OP_String8, 0, first_col + 5, 0, "", P4_STATIC);
sqlVdbeAddOp3(v, OP_MakeRecord, first_col + 1, 5, first_col);
return first_col;
@@ -1250,9 +1232,9 @@ sqlEndTable(struct Parse *pParse)
new_space->def->name);
sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
/* Do an insertion into _space_sequence. */
- int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
- reg_space_id, reg_seq_id,
- new_space->index[0]->def);
+ int reg_space_seq_record =
+ emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
+ reg_seq_id);
sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
reg_space_seq_record);
}
@@ -3222,7 +3204,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
}
int
-sql_add_autoincrement(struct Parse *parse_context, uint32_t field_id)
+sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno)
{
if (parse_context->create_table_def.has_autoinc) {
diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table "
@@ -3231,13 +3213,13 @@ sql_add_autoincrement(struct Parse *parse_context, uint32_t field_id)
return -1;
}
parse_context->create_table_def.has_autoinc = true;
- parse_context->create_table_def.autoinc_field_id = field_id;
+ parse_context->create_table_def.autoinc_fieldno = fieldno;
return 0;
}
int
-sql_field_id_by_name(struct Parse *parse_context, struct Expr *field_name,
- uint32_t *field_id)
+sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t *fieldno)
{
uint32_t i;
struct space_def *def = parse_context->create_table_def.new_space->def;
@@ -3247,19 +3229,15 @@ sql_field_id_by_name(struct Parse *parse_context, struct Expr *field_name,
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)
+ if (strcmp(def->fields[i].name, name->u.zToken) == 0)
break;
}
- /*
- * It is possible that i == def->field_count. It means
- * that field with AUTOINCREMENT wasn't described before
- * PRIMARY KEY() keywords. The error will be thrown later
- * in parser.
- */
- *field_id = i;
+ if (i == def->field_count) {
+ diag_set(ClientError, ER_SQL_CANT_RESOLVE_FIELD, name->u.zToken);
+ parse_context->is_aborted = true;
+ return -1;
+ }
+ *fieldno = i;
return 0;
}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 62cbf74..639f409 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -226,13 +226,13 @@ create_table_end ::= . { sqlEndTable(pParse); }
columnlist ::= columnlist COMMA tcons.
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)
+ uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 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)
+ uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0)
return;
}
columnlist ::= tcons.
@@ -767,29 +767,28 @@ sortlist(A) ::= expr(Y) sortorder(Z). {
}
/**
- * The sortlist_with_autoinc rule works the same as the sortlist
- * rule, except that it can work with the AUTOINCREMENT keyword.
+ * The sortlist_with_autoinc rule works in the same way as the
+ * sortlist rule, except that it can work with the AUTOINCREMENT
+ * keyword and cannot specify the sort order.
*/
%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)
+sortlist_with_autoinc(A) ::= sortlist_with_autoinc(A) COMMA expr(Y)
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))
+ uint32_t fieldno;
+ if (I == 1 && (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0 ||
+ sql_add_autoincrement(pParse, fieldno) != 0))
return;
A = sql_expr_list_append(pParse->db, A, Y.pExpr);
- sqlExprListSetSortOrder(A, Z);
}
-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))
+sortlist_with_autoinc(A) ::= expr(Y) autoinc(I). {
+ uint32_t fieldno;
+ if (I == 1 && (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0 ||
+ sql_add_autoincrement(pParse, fieldno) != 0))
return;
/* A-overwrites-Y. */
A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
- sqlExprListSetSortOrder(A, Z);
}
%type sortorder {int}
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 37deb9e..df1238b 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -215,7 +215,7 @@ struct create_table_def {
/** True, if table to be created has AUTOINCREMENT PK. */
bool has_autoinc;
/** Id of field with AUTOINCREMENT. */
- uint32_t autoinc_field_id;
+ uint32_t autoinc_fieldno;
};
struct create_view_def {
@@ -463,7 +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;
+ table_def->autoinc_fieldno = 0;
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 4421a48..de4a960 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4421,23 +4421,29 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_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.
+ * of PRIMARY KEY.
*
* @param parse_context Parsing context.
- * @param field_name Expr that contains field name.
- * @param field_id Field identifier.
+ * @param fieldno Field number in new space format.
*
* @retval 0 on success.
* @retval -1 on error.
*/
int
-sql_add_autoincrement(struct Parse *parse_context, uint32_t field_id);
+sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno);
+/**
+ * Get fieldno by field name.
+ *
+ * @param parse_context Parsing context.
+ * @param field_name Expr that contains field name.
+ * @param fieldno[out] Field number in new space format.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
int
-sql_field_id_by_name(struct Parse *parse_context, struct Expr *field_name,
- uint32_t *fieldno);
+sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t *fieldno);
#endif /* sqlINT_H */
diff --git a/test/sql-tap/where4.test.lua b/test/sql-tap/where4.test.lua
index 76e04cb..e389726 100755
--- a/test/sql-tap/where4.test.lua
+++ b/test/sql-tap/where4.test.lua
@@ -222,7 +222,7 @@ test:do_execsql_test(
INSERT INTO t2 VALUES(3);
-- Allow the 'x' syntax for backwards compatibility
- CREATE TABLE t4(x INT,y INT,z INT,PRIMARY KEY(x ASC, y ASC));
+ CREATE TABLE t4(x INT,y INT,z INT,PRIMARY KEY(x, y));
SELECT *
FROM t2 LEFT JOIN t4 b1
New patch:
>From 4cefd30183ea06d940c31836cc5e8b0aff7b3d11 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
Prior to this patch, the auto-increment feature could only be set
in an INTEGER field of PRIMARY KEY if the PRIMARY KEY consisted of
a single field. It was not possible to use this feature if the
PRIMARY KEY consisted of more than one field. This patch defines
two ways to set AUTOINCREMENT for any INTEGER or UNSIGNED field of
PRIMARY KEY.
Closes #4217
@TarantoolBot document
Title: The auto-increment feature for multipart PK
The auto-increment feature can be set to any INTEGER or UNSIGNED
field of PRIMARY KEY using one of two ways:
1) AUTOINCREMENT in column definition:
CREATE TABLE t (i INT, a INT AUTOINCREMENT, PRIMARY KEY (i, a));
CREATE TABLE t (i INT AUTOINCREMENT, a INT, PRIMARY KEY (i, a));
2) AUTOINCREMENT in PRIMARY KEY definition:
CREATE TABLE t (i INT, a INT, PRIMARY KEY (i, a AUTOINCREMENT));
CREATE TABLE t (i INT, a INT, PRIMARY KEY (i AUTOINCREMENT, a));
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index b4f114e..edaf5d0 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -521,12 +521,6 @@ sqlAddPrimaryKey(struct Parse *pParse)
sql_create_index(pParse);
if (db->mallocFailed)
goto primary_key_exit;
- } else if (pParse->create_table_def.has_autoinc) {
- diag_set(ClientError, ER_CREATE_SPACE, space->def->name,
- "AUTOINCREMENT is only allowed on an INTEGER PRIMARY "\
- "KEY or INT PRIMARY KEY");
- pParse->is_aborted = true;
- goto primary_key_exit;
} else {
sql_create_index(pParse);
pList = NULL;
@@ -942,18 +936,9 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
}
static int
-emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
- struct index_def *idx_def)
+emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id)
{
- struct key_part *part = &idx_def->key_def->parts[0];
- int fieldno = part->fieldno;
- char *path = NULL;
- if (part->path != NULL) {
- path = sqlDbStrNDup(pParse->db, part->path, part->path_len);
- if (path == NULL)
- return -1;
- path[part->path_len] = 0;
- }
+ uint32_t fieldno = pParse->create_table_def.autoinc_fieldno;
Vdbe *v = sqlGetVdbe(pParse);
int first_col = pParse->nMem + 1;
@@ -972,9 +957,7 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
sqlVdbeAddOp2(v, OP_Integer, fieldno, first_col + 4);
/* 5. Field path. */
- sqlVdbeAddOp4(v, OP_String8, 0, first_col + 5, 0,
- path != NULL ? path : "",
- path != NULL ? P4_DYNAMIC : P4_STATIC );
+ sqlVdbeAddOp4(v, OP_String8, 0, first_col + 5, 0, "", P4_STATIC);
sqlVdbeAddOp3(v, OP_MakeRecord, first_col + 1, 5, first_col);
return first_col;
@@ -1249,9 +1232,9 @@ sqlEndTable(struct Parse *pParse)
new_space->def->name);
sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
/* Do an insertion into _space_sequence. */
- int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
- reg_space_id, reg_seq_id,
- new_space->index[0]->def);
+ int reg_space_seq_record =
+ emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
+ reg_seq_id);
sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
reg_space_seq_record);
}
@@ -3219,3 +3202,42 @@ 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 fieldno)
+{
+ 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_fieldno = fieldno;
+ return 0;
+}
+
+int
+sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t *fieldno)
+{
+ 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;
+ }
+ for (i = 0; i < def->field_count; ++i) {
+ if (strcmp(def->fields[i].name, name->u.zToken) == 0)
+ break;
+ }
+ if (i == def->field_count) {
+ diag_set(ClientError, ER_SQL_CANT_RESOLVE_FIELD, name->u.zToken);
+ parse_context->is_aborted = true;
+ return -1;
+ }
+ *fieldno = i;
+ return 0;
+}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 643e025..639f409 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 fieldno = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0)
+ return;
+}
+columnlist ::= columnname carglist autoinc(I). {
+ uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1;
+ if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0)
+ return;
+}
columnlist ::= tcons.
columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
@@ -299,8 +307,7 @@ ccons ::= NULL onconf(R). {
sql_column_add_nullable_action(pParse, R);
}
ccons ::= NOT NULL onconf(R). {sql_column_add_nullable_action(pParse, R);}
-ccons ::= cconsname(N) PRIMARY KEY sortorder(Z) autoinc(I). {
- pParse->create_table_def.has_autoinc = I;
+ccons ::= cconsname(N) PRIMARY KEY sortorder(Z). {
create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
SQL_INDEX_TYPE_CONSTRAINT_PK, Z, false);
sqlAddPrimaryKey(pParse);
@@ -369,8 +376,7 @@ init_deferred_pred_opt(A) ::= . {A = 0;}
init_deferred_pred_opt(A) ::= INITIALLY DEFERRED. {A = 1;}
init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE. {A = 0;}
-tcons ::= cconsname(N) PRIMARY KEY LP sortlist(X) autoinc(I) RP. {
- pParse->create_table_def.has_autoinc = I;
+tcons ::= cconsname(N) PRIMARY KEY LP sortlist_with_autoinc(X) RP. {
create_index_def_init(&pParse->create_index_def, NULL, &N, X,
SQL_INDEX_TYPE_CONSTRAINT_PK, SORT_ORDER_ASC, false);
sqlAddPrimaryKey(pParse);
@@ -760,6 +766,31 @@ sortlist(A) ::= expr(Y) sortorder(Z). {
sqlExprListSetSortOrder(A,Z);
}
+/**
+ * The sortlist_with_autoinc rule works in the same way as the
+ * sortlist rule, except that it can work with the AUTOINCREMENT
+ * keyword and cannot specify the sort order.
+ */
+%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)
+ autoinc(I). {
+ uint32_t fieldno;
+ if (I == 1 && (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0 ||
+ sql_add_autoincrement(pParse, fieldno) != 0))
+ return;
+ A = sql_expr_list_append(pParse->db, A, Y.pExpr);
+}
+sortlist_with_autoinc(A) ::= expr(Y) autoinc(I). {
+ uint32_t fieldno;
+ if (I == 1 && (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0 ||
+ sql_add_autoincrement(pParse, fieldno) != 0))
+ return;
+ /* A-overwrites-Y. */
+ A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
+}
+
%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..df1238b 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_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_fieldno = 0;
}
static inline void
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index fbb3987..de4a960 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4419,4 +4419,31 @@ 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.
+ *
+ * @param parse_context Parsing context.
+ * @param fieldno Field number in new space format.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno);
+
+/**
+ * Get fieldno by field name.
+ *
+ * @param parse_context Parsing context.
+ * @param field_name Expr that contains field name.
+ * @param fieldno[out] Field number in new space format.
+ *
+ * @retval 0 on success.
+ * @retval -1 on error.
+ */
+int
+sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
+ uint32_t *fieldno);
+
#endif /* sqlINT_H */
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index 1d0be8a..83d62d7 100755
--- a/test/sql-tap/autoinc.test.lua
+++ b/test/sql-tap/autoinc.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(47)
+test:plan(56)
--!./tcltestrunner.lua
-- 2004 November 12
@@ -561,7 +561,7 @@ test:do_catchsql_test(
CREATE TABLE t8(x TEXT PRIMARY KEY AUTOINCREMENT);
]], {
-- <autoinc-7.2>
- 1, "Failed to create space 'T8': AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY or INT PRIMARY KEY"
+ 1, "Can't create or modify index 'pk_unnamed_T8_1' in space 'T8': sequence cannot be used with a non-integer key"
-- </autoinc-7.2>
})
@@ -814,4 +814,107 @@ test:do_catchsql_test(
-- </autoinc-gh-3670>
})
+--
+-- gh-4217: make sure that AUTOINCREMENT can be used for any
+-- INTEGER field of PRIMARY KEY.
+--
+
+test:do_execsql_test(
+ "autoinc-11.1",
+ [[
+ CREATE TABLE t11_1 (i INT AUTOINCREMENT, a INT, PRIMARY KEY(i, a));
+ INSERT INTO t11_1 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_1;
+ ]], {
+ -- <autoinc-11.1>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.1>
+ })
+
+test:do_execsql_test(
+ "autoinc-11.2",
+ [[
+ CREATE TABLE t11_2 (i INT AUTOINCREMENT, a INT, PRIMARY KEY(a, i));
+ INSERT INTO t11_2 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_2;
+ ]], {
+ -- <autoinc-11.2>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.2>
+ })
+
+test:do_execsql_test(
+ "autoinc-11.3",
+ [[
+ CREATE TABLE t11_3 (i INT, a INT, PRIMARY KEY(i AUTOINCREMENT, a));
+ INSERT INTO t11_3 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_3;
+ ]], {
+ -- <autoinc-11.3>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.3>
+ })
+
+test:do_execsql_test(
+ "autoinc-11.4",
+ [[
+ CREATE TABLE t11_4 (i INT, a INT, PRIMARY KEY(a, i AUTOINCREMENT));
+ INSERT INTO t11_4 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+ SELECT * FROM t11_4;
+ ]], {
+ -- <autoinc-11.4>
+ 1, 1, 2, 1, 3, 1
+ -- </autoinc-11.4>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.5",
+ [[
+ CREATE TABLE t11_5 (i INT, a INT, PRIMARY KEY(a, i COLLATE "unicode_ci" AUTOINCREMENT));
+ ]], {
+ -- <autoinc-11.5>
+ 1, "Wrong index options (field 2): collation is reasonable only for string and scalar parts"
+ -- </autoinc-11.5>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.6",
+ [[
+ CREATE TABLE t11_6 (i INT, a INT, b INT AUTOINCREMENT, PRIMARY KEY(a, i));
+ ]], {
+ -- <autoinc-11.6>
+ 1, "Can't create or modify index 'pk_unnamed_T11_6_1' in space 'T11_6': sequence field must be a part of the index"
+ -- </autoinc-11.6>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.7",
+ [[
+ CREATE TABLE t11_7 (i INT AUTOINCREMENT, a INT AUTOINCREMENT, PRIMARY KEY(a, i));
+ ]], {
+ -- <autoinc-11.7>
+ 1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
+ -- </autoinc-11.7>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.8",
+ [[
+ CREATE TABLE t11_8 (i INT, a INT, PRIMARY KEY(a AUTOINCREMENT, i AUTOINCREMENT));
+ ]], {
+ -- <autoinc-11.8>
+ 1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
+ -- </autoinc-11.8>
+ })
+
+test:do_catchsql_test(
+ "autoinc-11.8",
+ [[
+ CREATE TABLE t11_9 (i INT, PRIMARY KEY(a AUTOINCREMENT), a INT);
+ ]], {
+ -- <autoinc-11.8>
+ 1, "Can’t resolve field 'A'"
+ -- </autoinc-11.8>
+ })
+
test:finish_test()
diff --git a/test/sql-tap/sql-errors.test.lua b/test/sql-tap/sql-errors.test.lua
index 05788e1..cb89524 100755
--- a/test/sql-tap/sql-errors.test.lua
+++ b/test/sql-tap/sql-errors.test.lua
@@ -62,7 +62,7 @@ test:do_catchsql_test(
CREATE TABLE t5 (i TEXT PRIMARY KEY AUTOINCREMENT);
]], {
-- <sql-errors-1.5>
- 1,"Failed to create space 'T5': AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY or INT PRIMARY KEY"
+ 1,"Can't create or modify index 'pk_unnamed_T5_1' in space 'T5': sequence cannot be used with a non-integer key"
-- </sql-errors-1.5>
})
diff --git a/test/sql-tap/where4.test.lua b/test/sql-tap/where4.test.lua
index 76e04cb..e389726 100755
--- a/test/sql-tap/where4.test.lua
+++ b/test/sql-tap/where4.test.lua
@@ -222,7 +222,7 @@ test:do_execsql_test(
INSERT INTO t2 VALUES(3);
-- Allow the 'x' syntax for backwards compatibility
- CREATE TABLE t4(x INT,y INT,z INT,PRIMARY KEY(x ASC, y ASC));
+ CREATE TABLE t4(x INT,y INT,z INT,PRIMARY KEY(x, y));
SELECT *
FROM t2 LEFT JOIN t4 b1
More information about the Tarantool-patches
mailing list