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 0860522FF9 for ; Mon, 16 Jul 2018 09:40:30 -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 3MS1HGZynTgz for ; Mon, 16 Jul 2018 09:40:29 -0400 (EDT) Received: from smtp3.mail.ru (smtp3.mail.ru [94.100.179.58]) (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 46E2422B5B for ; Mon, 16 Jul 2018 09:40:29 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure References: <4f4e7934d0eac2f295adadb1e5572f8ff4dbf752.1531743627.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <69de35d4-3e7d-6f9f-8181-05b06c577e6b@tarantool.org> Date: Mon, 16 Jul 2018 16:40:26 +0300 MIME-Version: 1.0 In-Reply-To: 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 Thanks for the patch! See 6 comments below. On 16/07/2018 15:28, Kirill Shcherbatov wrote: > Get rid of is_primkey in Column structure as it become > redundant. Moved the last member coll with collation pointer > to field_def structure. Finally, dropped Column. > --- > src/box/field_def.c | 1 + > src/box/field_def.h | 2 ++ > src/box/sql/alter.c | 16 +++------- > src/box/sql/build.c | 79 +++++++++++++++++-------------------------------- > src/box/sql/resolve.c | 11 +++---- > src/box/sql/select.c | 43 ++++++++++----------------- > src/box/sql/sqliteInt.h | 23 +++++++------- > 7 files changed, 65 insertions(+), 110 deletions(-) > > diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c > index fe54e55..4f50988 100644 > --- a/src/box/sql/alter.c > +++ b/src/box/sql/alter.c > @@ -299,19 +299,11 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc) > 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); > - pNew->aCol = > - (Column *) sqlite3DbMallocZero(db, sizeof(Column) * nAlloc); 1. And why do you need nAlloc now? Please, do not hurry. It is not ok that you overlook such conspicuous things. > /* FIXME: pNew->zName = sqlite3MPrintf(db, "sqlite_altertab_%s", pTab->zName); */ > /* FIXME: if (!pNew->aCol || !pNew->zName) { */ > - if (!pNew->aCol) { > - assert(db->mallocFailed); > - goto exit_begin_add_column; > - } > - memcpy(pNew->aCol, pTab->aCol, sizeof(Column) * pNew->def->field_count); > for (i = 0; i < pNew->def->field_count; i++) { > - Column *pCol = &pNew->aCol[i]; > /* FIXME: pNew->def->name = sqlite3DbStrDup(db, pCol->zName); */ 2. What does this comment mean? 3. What about other fields? Please, use = field_def_default. > - pCol->coll = NULL; > + pNew->def->fields[i].coll = NULL; > pNew->def->fields[i].coll_id = COLL_NONE; > } > pNew->pSchema = db->pSchema; > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index ec0bc4b..a6f3559 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c> @@ -131,10 +141,10 @@ check_on_conflict_replace_entries(struct Table *table) > 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 && > - table->aCol[i].is_primkey == false) { > + !index_has_column(pk->aiColumn, pk->nColumn, i)) 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. > return true; > - } > } > /* Check all UNIQUE constraints. */ > for (struct Index *idx = table->pIndex; idx; idx = idx->pNext) { > @@ -915,21 +909,19 @@ sqlite3AddPrimaryKey(Parse * pParse, /* Parsing context */ > goto primary_key_exit; > } > const char *zCName = pCExpr->u.zToken; > - for (iCol = 0; > - iCol < (int)pTab->def->field_count; iCol++) { > - if (strcmp > - (zCName, > - pTab->def->fields[iCol].name) == 0) { > - pCol = &pTab->aCol[iCol]; > - pCol->is_primkey = 1; > + 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; > break; 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. > } > } > } > } > - assert(pCol == NULL || pCol == &pTab->aCol[iCol]); > - if (nTerm == 1 && pCol != NULL && > - (pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER) && > + if (nTerm == 1 && iCol != -1 && > + pTab->def->fields[iCol].type == FIELD_TYPE_INTEGER && > sortOrder != SORT_ORDER_DESC) { > assert(autoInc == 0 || autoInc == 1); > pTab->iPKey = iCol; > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index ceb7e34..745ae2f 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1811,25 +1811,15 @@ sqlite3ColumnsFromExprList(Parse * parse, ExprList * expr_list, Table *table) > { > /* Database connection */ > sqlite3 *db = parse->db; > - int i, j; /* Loop counters */ > u32 cnt; /* Index added to make the name unique */ > - Column *aCol, *pCol; /* For looping over result columns */ > - int nCol; /* Number of columns in the result set */ > Expr *p; /* Expression for a single result column */ > char *zName; /* Column name */ > int nName; /* Size of name in zName[] */ > Hash ht; /* Hash table of column names */ > > sqlite3HashInit(&ht); > - if (expr_list) { > - nCol = expr_list->nExpr; > - aCol = sqlite3DbMallocZero(db, sizeof(aCol[0]) * nCol); > - testcase(aCol == 0); > - } else { > - nCol = 0; > - aCol = NULL; > - } > - assert(nCol == (i16) nCol); > + uint32_t column_count = > + (expr_list != NULL) ? (uint32_t)expr_list->nExpr : 0; 6. Why do you need to enclose != NULL into the brackets? > /* > * This should be a table without resolved columns. > * sqlite3ViewGetColumnNames could use it to resolve