[tarantool-patches] Re: [PATCH] sql: add index_def to struct Index (fixed version #2)

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for the fixes, you have done great work! But see
35 new comments.

And I have pushed a commit with review fixes on the branch. Please,
look and squash.

> 
>  From 9fe52af99c305aaf381de3b5414ab5fb03f65817 Mon Sep 17 00:00:00 2001
> From: Ivan Koptelov <ivan.koptelov@tarantool.org>
> Date: Thu, 7 Jun 2018 19:42:16 +0300
> Subject: [PATCH] sql: add index_def to Index

1. Please, do not paste SMTP headers in the body.

2. Looks like in the diff below all tabs are turned into 4 spaces. Please
cope with it. Maybe, your IDE made it.

> 
> Now every sqlite struct Index is created with tnt struct
> index_def inside. This allows us to use tnt index_def
> in work with sqlite indexes in the same manner as with
> tnt index and is a step to remove sqlite Index with
> tnt index.
> Fields coll_array, coll_id_array, aiColumn, sort_order
> and zName are removed from Index. All usages of this
> fields changed to usage of corresponding index_def
> fields.
> index_is_unique(), sql_index_collation() and
> index_column_count() are removed with calls of
> index_def corresponding fields.
> 
> Closes: #3369
> Github branch: https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where

3. Please, put branch link below ---.

> ---

Here. And do not forget about link to the issue.

     Branch: <link>
     Issue: <link>

>   src/box/sql.c           |  18 +--
>   src/box/sql/analyze.c   |  24 +--
>   src/box/sql/build.c     | 398 ++++++++++++++++++++++++------------------------
>   src/box/sql/delete.c    |  16 +-
>   src/box/sql/expr.c      |  59 ++++---
>   src/box/sql/fkey.c      |  41 +++--
>   src/box/sql/insert.c    | 134 +++++++---------
>   src/box/sql/pragma.c    |  19 ++-
>   src/box/sql/select.c    |   2 +-
>   src/box/sql/sqliteInt.h |  25 +--
>   src/box/sql/trigger.c   |   2 -
>   src/box/sql/update.c    |  10 +-
>   src/box/sql/vdbemem.c   |   2 +-
>   src/box/sql/where.c     | 140 ++++++++---------
>   src/box/sql/wherecode.c |  43 +++---
>   src/box/sql/whereexpr.c |  15 --
>   16 files changed, 433 insertions(+), 515 deletions(-)
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 28e4d7a4d..74fb66565 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1072,11 +1075,9 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
>         * collation type was added. Correct this if it is the case.
>         */
>        for (pIdx = p->pIndex; pIdx; pIdx = pIdx->pNext) {
> -        assert(pIdx->nColumn == 1);
> -        if (pIdx->aiColumn[0] == i) {
> -           id = &pIdx->coll_id_array[0];
> -           pIdx->coll_array[0] =
> -              sql_column_collation(p->def, i, id);
> +        assert(pIdx->def->key_def->part_count == 1);
> +        if ((int)pIdx->def->key_def->parts[0].fieldno == i) {
> +           id = &pIdx->def->key_def->parts[0].coll_id;

4. I have just noticed the zColl leaks here. It is not deleted if
coll != NULL, but must.

5. Here you have removed coll * initialization.
sql_column_collation function here was used to set coll id and collation,
by column number. But now this whole cycle does nothing as you can see.
Please, return this initialization. You sill must init
key_def->parts[0].coll_id and coll.

>           }
>        }
>     } else {
> @@ -1123,52 +1124,10 @@ sql_index_key_def(struct Index *idx)
>   enum sort_order
>   sql_index_column_sort_order(Index *idx, uint32_t column)

6. Now this function is useless one line wrapper. Please,
remove it too.

>   {
> -  assert(idx != NULL);
> -  uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(idx->pTable->tnum);
> -  struct space *space = space_by_id(space_id);
> -
> -  assert(column < idx->nColumn);
> -  /*
> -   * If space is still under construction, or it is
> -   * an ephemeral space, then fetch collation from
> -   * SQL internal structure.
> -   */
> -  if (space == NULL) {
> -     assert(column < idx->nColumn);
> -     return idx->sort_order[column];
> -  }
> -
> -  struct key_def *key_def = sql_index_key_def(idx);
> -  assert(key_def != NULL && key_def->part_count >= column);
> -  return key_def->parts[column].sort_order;
> +  return idx->def->key_def->parts[column].sort_order;
>   }
>   
>   /**
> @@ -1383,14 +1342,16 @@ createTableStmt(sqlite3 * db, Table * p)
>     return zStmt;
>   }
>   
> -/* Return true if value x is found any of the first nCol entries of aiCol[]
> - */
>   static int
> -hasColumn(const i16 * aiCol, int nCol, int x)
> +hasColumn(const struct key_part *key_parts, int nCol, const struct key_part key_part)

7. Passing a struct by value is very bad idea. Please, don't do it ever.
And looks like here fieldno is enough.

>   {
> -  while (nCol-- > 0)
> -     if (x == *(aiCol++))
> +  int i = 0;
> +  while (i < nCol) {
> +     if (key_part.fieldno == key_parts->fieldno)
>           return 1;
> +     key_parts++;
> +     i++;
> +  }
>     return 0;
>   }
>   
> @@ -1410,13 +1371,13 @@ static void
>   convertToWithoutRowidTable(Parse * pParse, Table * pTab)
>   {
>     Index *pPk;
> -  int i, j;
> +  uint32_t i, j;

8. Please, do not predeclare cycle iterators when possible. It is
SQLite code style, not Tarantool. When you change the SQLite code,
it must turn into Tarantool style.

> @@ -1454,14 +1415,17 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
>         * "PRIMARY KEY(a,b,a,b,c,b,c,d)" into just "PRIMARY KEY(a,b,c,d)".  Later
>         * code assumes the PRIMARY KEY contains no repeated columns.
>         */
> -     for (i = j = 1; i < pPk->nColumn; i++) {
> -        if (hasColumn(pPk->aiColumn, j, pPk->aiColumn[i])) {
> -           pPk->nColumn--;
> +     for (i = j = 1; i < pPk->def->key_def->part_count; i++) {
> +        if (hasColumn(pPk->def->key_def->parts, j,
> +              pPk->def->key_def->parts[i])) {
> +           pPk->def->key_def->part_count--;
>           } else {
> -           pPk->aiColumn[j++] = pPk->aiColumn[i];
> +           pPk->def->key_def->parts[j++] =
> +                 pPk->def->key_def->parts[i];
>           }

9. Wrong alignments almost on all new lines. And please, save a
key_def->parts in a separate variable and use it instead of the full
path pPk->def->key_def->parts. It is too long to be used 4 times on
6 lines.

10. I see that cycle iterates until pPk->def->key_def->part_count,
but this variable is decremented inside the cycle. So the last
columns are not checked. This value must be saved in a separate
variable before usage as a 'for' guard.

>        }
> -     pPk->nColumn = j;
> +
> +     pPk->def->key_def->part_count = j;

11. This line makes no sense. You have already updated part_count
inside the cycle. Either you update here, or in the cycle.

>     }
>     assert(pPk != 0);
>   }
> @@ -2654,8 +2618,9 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
>     }
>     /* Open the sorter cursor if we are to use one. */
>     iSorter = pParse->nTab++;
> -  sqlite3VdbeAddOp4(v, OP_SorterOpen, iSorter, 0, pIndex->nColumn,
> -          (char *)def, P4_KEYDEF);
> +  sqlite3VdbeAddOp4(v, OP_SorterOpen, iSorter, 0,
> +        pIndex->def->key_def->part_count,
> +        (char *)def, P4_KEYDEF);

12. Wrong alignment.

>   
>     /* Open the table. Loop through all rows of the table, inserting index
>      * records into the sorter.
> @@ -2687,7 +2652,7 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
>        sqlite3VdbeGoto(v, j2);
>        addr2 = sqlite3VdbeCurrentAddr(v);
>        sqlite3VdbeAddOp4Int(v, OP_SorterCompare, iSorter, j2,
> -                regRecord, pIndex->nColumn);
> +           regRecord, pIndex->def->key_def->part_count);

13. Same.

>        VdbeCoverage(v);
>        sqlite3UniqueConstraint(pParse, ON_CONFLICT_ACTION_ABORT,
>                 pIndex);
> @@ -2733,16 +2698,11 @@ sqlite3AllocateIndexObject(sqlite3 * db,   /* Database connection */
>     p = sqlite3DbMallocZero(db, nByte + nExtra);
>     if (p) {
>        char *pExtra = ((char *)p) + ROUND8(sizeof(Index));
> -     p->coll_array = (struct coll **)pExtra;

14. I still see coll_array in build.c in comments.

>        pExtra += ROUND8(sizeof(struct coll **) * nCol);
> -     p->coll_id_array = (uint32_t *) pExtra;

15. Same.

>        pExtra += ROUND8(sizeof(uint32_t) * nCol);
>        p->aiRowLogEst = (LogEst *) pExtra;
>        pExtra += sizeof(LogEst) * (nCol + 1);
> -     p->aiColumn = (i16 *) pExtra;

16. Same. And in very many places.

>        pExtra += sizeof(i16) * nCol;
> -     p->sort_order = (enum sort_order *) pExtra;
> -     p->nColumn = nCol;
>        *ppExtra = ((char *)p) + nByte;

17. You have removed the fields, but did not update
sqlite3AllocateIndexObject that still allocates memory for them.

>     }
>     return p;
> @@ -2831,18 +2791,119 @@ addIndexToTable(Index * pIndex, Table * pTab)
>     }
>   }
>   
> -bool
> -index_is_unique(Index *idx)
> +void
> +append(struct region *r, const char *str, size_t *total_sql_size)
>   {
> -  assert(idx != NULL);
> -  uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(idx->tnum);
> -  uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum);
> -  struct space *space = space_by_id(space_id);
> -  assert(space != NULL);
> -  struct index *tnt_index = space_index(space, index_id);
> -  assert(tnt_index != NULL);
> +  memcpy(region_alloc(r, strlen(str)), str, strlen(str));
> +  *total_sql_size += strlen(str);
> +}
18. Please, rename append function. This name is too common. And
make it static inline.

19. You do not check region_alloc fails.

> +
> +char *
> +create_sql(const char *idx_name, struct space_def *space_def, ExprList *expr_list)

20. Same as 18.

21. Out of 80 symbols.

22. Please, use struct ExprList, not ExprList.

23. I still can not understand why can not you merge this function into
index_def building. It is needed in a single place, and makes redundant
scan of ExprList.

> +{
> +  struct region *r = &fiber()->gc;
> +  size_t total_sql_size = 0;
> +  append(r, "CREATE INDEX ", &total_sql_size);
> +  append(r, idx_name, &total_sql_size);
> +  append(r, " ON ", &total_sql_size);
> +  append(r, space_def->name, &total_sql_size);
> +  append(r, " (", &total_sql_size);
> +
> +  for (int i = 0; i < expr_list->nExpr; i++){
> +     Expr *expr = expr_list->a[i].pExpr;
> +     assert(expr->op == TK_COLLATE || expr->op == TK_COLUMN);
> +     Expr *column_expr = sqlite3ExprSkipCollate(expr);
> +     const char *name = space_def->fields[column_expr->iColumn].name;
> +
> +     if (expr->op == TK_COLLATE){
> +        append(r, name, &total_sql_size);
> +        append(r, " COLLATE ", &total_sql_size);
> +        const char *coll_name = expr->u.zToken;
> +        append(r, coll_name, &total_sql_size);
> +        append(r, ", ", &total_sql_size);
> +     } else {
> +        append(r, name, &total_sql_size);
> +        append(r, ", ", &total_sql_size);
> +     }
> +  }
> +
> +  memcpy(region_alloc(r, 1), "\0", 1);
> +  total_sql_size += 1;
> +  char *res = region_join(r, total_sql_size);
> +
> +  /*
> +   * fix last ", " with ")\0"
> +   */
> +  res[strlen(res) - 2] = ')';
> +  res[strlen(res) - 1] = '\0';

24. strlen has O(N) complexity. And here you already know the
result: total_sql_size.

> +  return res;
> +}
> +
> +void
> +set_index_def(Parse *parse, Index *index, Table *table, uint32_t iid,
> +     const char *name, uint32_t name_len, int on_error,
> +     ExprList *expr_list, u8 idx_type)

25. Still bad alignment. And make this function be static.


> +{
> +  struct space_def *space_def = table->def;
> +  struct index_opts opts;
> +  index_opts_create(&opts);
> +  opts.is_unique = on_error != ON_CONFLICT_ACTION_NONE;
> +
> +  struct key_def *key_def = key_def_new(expr_list->nExpr);
> +  if (key_def == NULL)

26. If key_def_new fails, you should inform the parser or
db: either set Parse.nErr and Parser.rc, or sqlite3OomFaul(). Same
about all other errors got from Tarantool functions.

> +     return;
> +
> +  for (int i = 0; i < expr_list->nExpr; i++) {
> +     Expr *expr = expr_list->a[i].pExpr;
> +     sql_resolve_self_reference(parse, table, NC_IdxExpr,
> +              expr, 0);

27. Bad alignment. And as I can see it fits in one line.

Please check alignment during another self-review iteration in
the whole patch. I will not mention it below again.

> +     if (parse->nErr > 0)
> +        return;
> +
> +     Expr *column_expr = sqlite3ExprSkipCollate(expr);
> +     if (column_expr->op != TK_COLUMN) {
> +        sqlite3ErrorMsg(parse,
> +              "functional indexes aren't supported "
> +              "in the current version");
> +        return;
> +     }
> +
> +     uint32_t fieldno = column_expr->iColumn;
> +
> +     uint32_t coll_id;
> +     struct coll *coll;
> +     if (expr->op == TK_COLLATE)
> +        coll = sql_get_coll_seq(parse, expr->u.zToken,
> +                 &coll_id);
> +     else
> +        coll = sql_column_collation(space_def, fieldno,
> +                 &coll_id);

28. Please, use {}, when 'if' body consists of multiple lines.

> +
> +     if (sqlite3StrICmp(expr->u.zToken, "binary") != 0 &&
> +         coll == NULL && expr->op == TK_COLLATE)

29. This check is needed in 'if (expr->op == TK_COLLATE)' above.
And why do you do this youself? sql_get_coll_seq already sets
the nErr in parser for this error.

> +        return;
> +
> +     /* Tarantool: DESC indexes are not supported so far.
> +     * See gh-3016.

30. Please, obey Tarantool comment style:
/*
  * My comment inside the function.
  * Second line.
  */

And wrap the comment line on 66 ruler.

>   void
> @@ -2853,12 +2914,11 @@ sql_create_index(struct Parse *parse, struct Token *token,
>   {
>     Table *pTab = 0;   /* Table to be indexed */
>     Index *pIndex = 0; /* The index to be created */
> -  char *zName = 0;   /* Name of the index */
> +  char *name = 0;    /* Name of the index */

31. Please, use explicit NULL.

>     int nName;    /* Number of characters in zName */
> -  int i, j;
> +  int i;
>     DbFixer sFix;     /* For assigning database names to pTable */
>     sqlite3 *db = parse->db;
> -  struct ExprList_item *col_listItem;    /* For looping over col_list */
>     int nExtra = 0;       /* Space allocated for zExtra[] */
>     char *zExtra = 0;  /* Extra space after the Index object */
>     struct session *user_session = current_session();
> @@ -3159,6 +3189,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
>              }
>              if (idx_type == SQLITE_IDXTYPE_PRIMARYKEY)
>                 pIdx->idxType = idx_type;
> +
>              goto exit_create_index;

32. Garbage diff. Please, find other garbage diffs and remove them.

>           }
>        }
> @@ -3268,28 +3299,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
>     sql_expr_delete(db, where, false);
>     sql_expr_list_delete(db, col_list);
>     sqlite3SrcListDelete(db, tbl_name);

33. I have noticed than on line 3056 collation names add extra bytes
to struct Index object. Can you please investigate if they are not
needed anymore and remove this code?

I am talking about it:

	/* Figure out how many bytes of space are required to store explicitly
	 * specified collation sequence names.
	 */
	for (i = 0; i < col_list->nExpr; i++) {
		Expr *pExpr = col_list->a[i].pExpr;
		assert(pExpr != 0);
		if (pExpr->op == TK_COLLATE) {
			nExtra += (1 + sqlite3Strlen30(pExpr->u.zToken));
		}
	}

We do not store collation names in struct Index anymore.

> @@ -3297,15 +3307,8 @@ bool
>   index_is_unique_not_null(const Index *idx)
>   {
>     assert(idx != NULL);
> -  uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(idx->tnum);
> -  struct space *space = space_by_id(space_id);
> -  assert(space != NULL);
> -
> -  uint32_t index_id = SQLITE_PAGENO_TO_INDEXID(idx->tnum);
> -  struct index *index = space_index(space, index_id);
> -  assert(index != NULL);
> -  return (index->def->opts.is_unique &&
> -     !index->def->key_def->is_nullable);
> +  assert(idx->def != NULL);
> +  return (idx->def->key_def->is_nullable && idx->def->opts.is_unique);

34. This one-line function is used in a single place. Please, inline
it and remove.

>   }
>   
>   void
> @@ -3933,18 +3936,18 @@ sqlite3UniqueConstraint(Parse * pParse,    /* Parsing context */
>       )
>   {
>     char *zErr;
> -  int j;
> +  uint32_t j;
>     StrAccum errMsg;
>     Table *pTab = pIdx->pTable;
>   
>     sqlite3StrAccumInit(&errMsg, pParse->db, 0, 0, 200);
>     if (pIdx->aColExpr) {
> -     sqlite3XPrintf(&errMsg, "index '%q'", pIdx->zName);
> +     sqlite3XPrintf(&errMsg, "index '%q'", pIdx->def->name);
>     } else {
> -     for (j = 0; j < pIdx->nColumn; j++) {
> +     for (j = 0; j < pIdx->def->key_def->part_count; j++) {
>           char *zCol;
> -        assert(pIdx->aiColumn[j] >= 0);
> -        zCol = pTab->def->fields[pIdx->aiColumn[j]].name;
> +        uint32_t fieldno = pIdx->def->key_def->parts[j].fieldno;
> +        zCol = pTab->def->fields[fieldno].name;
>           if (j)
>              sqlite3StrAccumAppend(&errMsg, ", ", 2);
>           sqlite3XPrintf(&errMsg, "%s.%s", pTab->def->name, zCol);
> @@ -3967,11 +3970,10 @@ static bool
>   collationMatch(struct coll *coll, struct Index *index)
>   {
>     assert(coll != NULL);
> -  for (int i = 0; i < index->nColumn; i++) {
> -     uint32_t id;
> -     struct coll *idx_coll = sql_index_collation(index, i, &id);
> -     assert(idx_coll != 0 || index->aiColumn[i] < 0);
> -     if (index->aiColumn[i] >= 0 && coll == idx_coll)
> +  for (uint32_t i = 0; i < index->def->key_def->part_count; i++) {
> +     struct coll *idx_coll = index->def->key_def->parts[i].coll;
> +     assert(idx_coll != NULL);
> +     if (coll == idx_coll)
>           return true;
>     }
>     return false;
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index ddad54b3e..504738cd5 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -252,11 +252,11 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>        /* Extract the primary key for the current row */
>        if (!is_view) {
>           for (int i = 0; i < pk_len; i++) {
> -           assert(pk->aiColumn[i] >= 0);
>              sqlite3ExprCodeGetColumnOfTable(v, table->def,
>                          tab_cursor,
> -                       pk->
> -                       aiColumn[i],
> +                       pk->def->
> +                       key_def->
> +                       parts[i].fieldno,

35. Please, just save pk->def->key_def->parts above in a separate
variable, and use here only part->fieldno and ++part above.


Please, fix the comments above, and find and fix the same problems in
the other code. Then the next review will be.

>                          reg_pk + i);
>           }
>        } else {