[tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon Jul 16 16:40:26 MSK 2018
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
More information about the Tarantool-patches
mailing list