From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CAFAC43D678 for ; Mon, 14 Oct 2019 15:28:20 +0300 (MSK) Date: Mon, 14 Oct 2019 15:28:17 +0300 From: Nikita Pettik Message-ID: <20191014122810.GA20746@tarantool.org> References: <35811B45-3F54-4D97-9D1C-E1437A043949@tarantool.org> <20190828114729.GA3245@tarantool.org> <20190918172225.GB89644@tarantool.org> <20190921072240.GA9659@tarantool.org> <20190925110445.GA32291@tarantool.org> <20190926132044.GA10775@tarantool.org> <20190930135607.GA58871@tarantool.org> <20191009143056.GA12954@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20191009143056.GA12954@tarantool.org> Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT List-Id: Tarantool development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@freelists.org, tarantool-patches@dev.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; ]], { - -- 1, 1, 2, 1, 3, 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; ]], { - -- 1, 1, 2, 1, 3, 1 - -- }) 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; ]], { - -- 1, 1, 2, 1, 3, 1 - -- }) 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; ]], { - -- 1, 1, 2, 1, 3, 1 - -- }) 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)); ]], { - -- 1, "Wrong index options (field 2): collation is reasonable only for string and scalar parts" - -- }) 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)); ]], { - -- 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" - -- }) 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)); ]], { - -- 1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field" - -- }) 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)); ]], { - -- 1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field" - -- }) test:do_catchsql_test( - "autoinc-11.8", + "autoinc-11.9", [[ CREATE TABLE t11_9 (i INT, PRIMARY KEY(a AUTOINCREMENT), a INT); ]], { - -- 1, "Can’t resolve field 'A'" - -- }) +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" +}) +