From: Nikita Pettik <korablev@tarantool.org> To: Mergen Imeev <imeevma@tarantool.org> Cc: tarantool-patches@freelists.org, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT Date: Mon, 14 Oct 2019 15:28:17 +0300 [thread overview] Message-ID: <20191014122810.GA20746@tarantool.org> (raw) In-Reply-To: <20191009143056.GA12954@tarantool.org> On 09 Oct 17:30, Mergen Imeev wrote: > Hi! Thank you for review. My answers, diff and new patch > below. Pushed with following code-style changes: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index edaf5d09c..233f56734 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -3221,7 +3221,6 @@ 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) { @@ -3229,6 +3228,7 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name, parse_context->is_aborted = true; return -1; } + uint32_t i; for (i = 0; i < def->field_count; ++i) { if (strcmp(def->fields[i].name, name->u.zToken) == 0) break; diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index 639f409ae..ed59a875a 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -230,6 +230,7 @@ columnlist ::= columnlist COMMA columnname carglist autoinc(I). { 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) @@ -376,7 +377,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_with_autoinc(X) RP. { +tcons ::= cconsname(N) PRIMARY KEY LP col_list_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); @@ -767,26 +768,32 @@ sortlist(A) ::= expr(Y) sortorder(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. + * Non-terminal rule to store a list of columns within PRIMARY KEY + * declaration. */ -%type sortlist_with_autoinc {ExprList*} -%destructor sortlist_with_autoinc {sql_expr_list_delete(pParse->db, $$);} +%type col_list_with_autoinc {ExprList*} +%destructor col_list_with_autoinc {sql_expr_list_delete(pParse->db, $$);} -sortlist_with_autoinc(A) ::= sortlist_with_autoinc(A) COMMA expr(Y) +col_list_with_autoinc(A) ::= col_list_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; + if (I == 1) { + if (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0) + return; + if (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; + +col_list_with_autoinc(A) ::= expr(Y) autoinc(I). { + if (I == 1) { + uint32_t fieldno = 0; + if (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0) + return; + if (sql_add_autoincrement(pParse, fieldno) != 0) + return; + } /* A-overwrites-Y. */ A = sql_expr_list_append(pParse->db, NULL, Y.pExpr); } diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index de4a96030..35fc81dfb 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -4424,16 +4424,20 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name, * of PRIMARY KEY. * * @param parse_context Parsing context. - * @param fieldno Field number in new space format. + * @param fieldno Field number in space format under construction. * * @retval 0 on success. - * @retval -1 on error. + * @retval -1 if table already has declared AUTOINCREMENT feature. */ int sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno); /** - * Get fieldno by field name. + * Get fieldno by field name. At the moment of forming space format + * there's no tuple dictionary, so we can't use hash, in contrast to + * tuple_fieldno_by_name(). However, AUTOINCREMENT can occur at most + * once in table's definition, so it's not a big deal if we use O(n) + * search. * * @param parse_context Parsing context. * @param field_name Expr that contains field name. diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua index 83d62d723..39e47966b 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(56) +test:plan(57) --!./tcltestrunner.lua -- 2004 November 12 @@ -826,9 +826,7 @@ test:do_execsql_test( 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( @@ -838,9 +836,7 @@ test:do_execsql_test( 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( @@ -850,9 +846,7 @@ test:do_execsql_test( 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( @@ -862,9 +856,7 @@ test:do_execsql_test( 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( @@ -872,9 +864,7 @@ test:do_catchsql_test( [[ 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( @@ -882,9 +872,7 @@ test:do_catchsql_test( [[ 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( @@ -892,9 +880,7 @@ test:do_catchsql_test( [[ 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( @@ -902,19 +888,23 @@ test:do_catchsql_test( [[ 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", + "autoinc-11.9", [[ 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:do_catchsql_test( + "autoinc-11.10", + [[ + CREATE TABLE t11_8 (i INT, a INT, PRIMARY KEY(a, 1 AUTOINCREMENT)); + ]], { + 1, "Expressions are prohibited in an index definition" +}) +
prev parent reply other threads:[~2019-10-14 12:28 UTC|newest] Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-27 10:29 imeevma 2019-08-09 14:48 ` [tarantool-patches] " n.pettik 2019-08-28 11:47 ` Mergen Imeev [not found] ` <20190918172225.GB89644@tarantool.org> [not found] ` <20190921072240.GA9659@tarantool.org> [not found] ` <20190925110445.GA32291@tarantool.org> [not found] ` <20190926132044.GA10775@tarantool.org> [not found] ` <20190930135607.GA58871@tarantool.org> [not found] ` <20191009143056.GA12954@tarantool.org> 2019-10-14 12:28 ` Nikita Pettik [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20191014122810.GA20746@tarantool.org \ --to=korablev@tarantool.org \ --cc=imeevma@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='Re: [Tarantool-patches] [tarantool-patches] [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox