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 CFC1426DF6 for ; Tue, 17 Jul 2018 05:33:02 -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 aELEwKmP62wL for ; Tue, 17 Jul 2018 05:33:02 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 8C83C26D34 for ; Tue, 17 Jul 2018 05:33:01 -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> <9b1796b0-744a-4cc2-1011-9e6a091a6e90@tarantool.org> From: Vladislav Shpilevoy Message-ID: <5a90417c-f742-7a07-dc3d-a3a0646ec05f@tarantool.org> Date: Tue, 17 Jul 2018 12:32:57 +0300 MIME-Version: 1.0 In-Reply-To: <9b1796b0-744a-4cc2-1011-9e6a091a6e90@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed 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: Kirill Shcherbatov , tarantool-patches@freelists.org Hello. Thanks for the patch! But it again is raw. You skipped field_def.coll initialization for non-sql code, alter.cc etc. On 16/07/2018 19:33, Kirill Shcherbatov wrote: > 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. >