[tarantool-patches] Re: [PATCH v1 4/4] sql: get rid of Column structure

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx>, tarantool-patches@xxxxxxxxxxxxx
  • Date: Mon, 16 Jul 2018 16:40:26 +0300

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

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018