From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D9E1923C19 for ; Mon, 16 Jul 2018 12:33:21 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id vwPMtQK9QyLk for ; Mon, 16 Jul 2018 12:33:21 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 94EC822BFE for ; Mon, 16 Jul 2018 12:33:21 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure References: <4f4e7934d0eac2f295adadb1e5572f8ff4dbf752.1531743627.git.kshcherbatov@tarantool.org> <69de35d4-3e7d-6f9f-8181-05b06c577e6b@tarantool.org> From: Kirill Shcherbatov Message-ID: <9b1796b0-744a-4cc2-1011-9e6a091a6e90@tarantool.org> Date: Mon, 16 Jul 2018 19:33:18 +0300 MIME-Version: 1.0 In-Reply-To: <69de35d4-3e7d-6f9f-8181-05b06c577e6b@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Vladislav Shpilevoy Thank you for review. > 1. And why do you need nAlloc now? Please, do not hurry. It is not ok that > you overlook such conspicuous things. - nAlloc = (((pNew->def->field_count - 1) / 8) * 8) + 8; - assert((uint32_t)nAlloc >= pNew->def->field_count && nAlloc % 8 == 0 && - nAlloc - pNew->def->field_count < 8); Dropped. > 2. What does this comment mean? > 3. What about other fields? Please, use = field_def_default. - for (i = 0; i < pNew->def->field_count; i++) { - /* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */ - pNew->def->fields[i].coll = NULL; - pNew->def->fields[i].coll_id = COLL_NONE; - } + for (uint32_t i = 0; i < pNew->def->field_count; i++) + pNew->def->fields[i] = field_def_default; > 4. No. As I said you on the first review, you should not use here > scans or is_primkey. This check can be spread over other column check > functions, involved in the first commit with no additional scans. diff --git a/src/box/sql/build.c b/src/box/sql/build.c index a6f3559..3449d3c 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -126,38 +126,48 @@ index_has_column(const i16 *parts, int parts_count, int column_idx) /** * This is a function which should be called during execution - * of sqlite3EndTable. It ensures that only PRIMARY KEY - * constraint may have ON CONFLICT REPLACE clause. + * of sqlite3EndTable. It set defaults for columns having no + * separate NULL/NOT NULL specifiers and ensures that only + * PRIMARY KEY constraint may have ON CONFLICT REPLACE clause. * + * @param parser SQL Parser object. * @param table Space which should be checked. - * @retval False, if only primary key constraint has - * ON CONFLICT REPLACE clause or if there are no indexes - * with REPLACE as error action. True otherwise. + * @retval -1 on error. Parser SQL_TARANTOOL_ERROR is set. + * @retval 0 on success. */ -static bool -check_on_conflict_replace_entries(struct Table *table) -{ - /* Check all NOT NULL constraints. */ - for (int i = 0; i < (int)table->def->field_count; i++) { - enum on_conflict_action on_error = - table->def->fields[i].nullable_action; - struct Index *pk = sqlite3PrimaryKeyIndex(table); - if (on_error == ON_CONFLICT_ACTION_REPLACE && +static int +check_on_conflict_replace_entries(struct Parse *parser, struct Table *table) +{ + const char *err_msg = NULL; + struct field_def *field = table->def->fields; + struct Index *pk = sqlite3PrimaryKeyIndex(table); + for (uint32_t i = 0; i < table->def->field_count; ++i, ++field) { + if (field->nullable_action == on_conflict_action_MAX) { + /* Set default. */ + field->nullable_action = ON_CONFLICT_ACTION_NONE; + field->is_nullable = true; + } + if (field->nullable_action == ON_CONFLICT_ACTION_REPLACE && !index_has_column(pk->aiColumn, pk->nColumn, i)) - return true; + goto non_pk_on_conflict_error; } - /* Check all UNIQUE constraints. */ + for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) { if (idx->onError == ON_CONFLICT_ACTION_REPLACE && - !IsPrimaryKeyIndex(idx)) { - return true; - } + !IsPrimaryKeyIndex(idx)) + goto non_pk_on_conflict_error; } - /* - * CHECK constraints are not allowed to have REPLACE as - * error action and therefore can be skipped. - */ - return false; + + return 0; + +non_pk_on_conflict_error: + err_msg = tt_sprintf("only PRIMARY KEY constraint can have " + "ON CONFLICT REPLACE clause - %s", + table->def->name); + diag_set(ClientError, ER_SQL, err_msg); + parser->rc = SQL_TARANTOOL_ERROR; + parser->nErr++; + return -1; } /* @@ -1748,22 +1758,8 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ } } - /* Set default on_nullable action if required. */ - struct field_def *field = p->def->fields; - for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) { - if (field->nullable_action == on_conflict_action_MAX) { - field->nullable_action = ON_CONFLICT_ACTION_NONE; - field->is_nullable = true; - } - } - - if (check_on_conflict_replace_entries(p)) { - sqlite3ErrorMsg(pParse, - "only PRIMARY KEY constraint can " - "have ON CONFLICT REPLACE clause " - "- %s", p->def->name); + if (check_on_conflict_replace_entries(pParse, p)) goto cleanup; - } if (db->init.busy) { /* @@ -1906,6 +1902,7 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ cleanup: sql_expr_list_delete(db, p->def->opts.checks); p->def->opts.checks = NULL; + return; } > 5. Please, make the code more readable. You should avoid such long names > as 'collumn_id' (btw 'collumn' word does not exist), save pTab->def->fields > in a separate variable etc. @@ -905,16 +915,15 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ sqlite3ExprSkipCollate(pList->a[i].pExpr); assert(pCExpr != 0); if (pCExpr->op != TK_ID) { - sqlite3ErrorMsg(pParse, "expressions prohibited in PRIMARY KEY"); + sqlite3ErrorMsg(pParse, "expressions prohibited" + " in PRIMARY KEY"); goto primary_key_exit; } - const char *zCName = pCExpr->u.zToken; - for (uint32_t collumn_id = 0; - collumn_id < pTab->def->field_count; collumn_id++) { - if (strcmp(zCName, - pTab->def->fields[collumn_id].name) - == 0) { - iCol = collumn_id; + const char *name = pCExpr->u.zToken; + struct space_def *def = pTab->def; + for (uint32_t idx = 0; idx < def->field_count; idx++) { + if (strcmp(name, def->fields[idx].name) == 0) { + iCol = idx; break; } } > 6. Why do you need to enclose != NULL into the brackets? - (expr_list != NULL) ? (uint32_t)expr_list->nExpr : 0; + expr_list != NULL ? (uint32_t)expr_list->nExpr : 0; Hm, It's is a little more convenient to read for me. I'll try to be a little more pedantic.