[tarantool-patches] Re: [PATCH] sql: add index_def to struct Index (fixed version)
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon May 28 15:10:46 MSK 2018
Hello. Thanks for the patch! See my 35 comments below.
1. The patch is not so big to do not fit in a letter. Please, send a patch as a file only when
it is really huge.
2. There is a problem with your GibHub account. Your profile name in commits on github must
be a link to the profile. But your name is just a text. Please, cope with the problem.
3. Now you can remove sql_index_collation() function. It is useless two line wrapper. And
its second out parameter is not needed in some places.
4. Same about index_is_unique().
5. get_new_iid() is unused.
> +void
> +append(struct region *r, const char *str, char **sql_arr, int idx, int total_sql_str_size)
> +{
> + sql_arr[idx] = region_alloc(r, strlen(str));
> + strcpy(sql_arr[idx], str);
> + total_sql_str_size += strlen(str);
> +}
6. How does it work? total_sql_str_size is not out parameter here. In the caller
context it is still unchanged.
> +
> +char *
> +create_sql(const char *idx_name, struct space_def *space_def, ExprList *expr_list)
> +{
> + struct region *r = &fiber()->gc;
> + /*
> + * CREATE_INDEX_ + INDEX_NAME + _ON_ + TABLE_NAME + _(
> + */
> + uint32_t sql_parts_count = 5;
7. You do not need sql parts array, part count. You just do region_alloc + memcpy and
region_join at the end. It is not required to store each part anywhere.
> + Expr *column_expr;
> + for (uint32_t i = 0; i < (uint32_t) expr_list->nExpr; i++){
> + column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr);
> + sql_parts_count += expr_list->a[i].pExpr == column_expr ? 2 : 4;
> + }
> +
> + size_t total_sql_str_size = 0;
> +
> + char **sql = region_alloc(r, sizeof(char*) * sql_parts_count);
> +
> + char *CREATE_INDEX = "CREATE INDEX ";
8. Do not store const string as non-const.
9. Why do you need this variable? Why can not you just
inline it one line below?.
> + append(r, CREATE_INDEX, sql, 0, total_sql_str_size);
> + append(r, idx_name, sql, 1, total_sql_str_size);
> +
> + char *ON = " ON ";
10. Same.
> + append(r, ON, sql, 2, total_sql_str_size);
> +
> + char *name = space_def->name;
> + append(r, name, sql, 3, total_sql_str_size);
> + append(r, " (", sql, 4, total_sql_str_size);
> +
> + Expr *collation_expr;
> + uint32_t sql_idx = 5;
> + for (int i = 0; i < expr_list->nExpr; i++){
> + column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr);
> + collation_expr = expr_list->a[i].pExpr == column_expr ?
> + NULL : expr_list->a[i].pExpr;
11. Please, do like sql_create_index(): use pExpr->op == TK_COLLATE to detect that
it is a collated column.
12. Why did not you call sqlite3ResolveSelfReference() ?
13. Why your patch did not delete Index.coll_array, Index.coll_id_array, Index.aiColumn?
Your patch must remove them. Index_def initialization must replace code in build.c
on lines 3158:3202.
> + // fix last ", " with ")\0"
> + strcpy(sql[sql_idx - 1], ")\0");
> + char *res = region_join(r, total_sql_str_size);
> + return res;
14. Do not use // comments.
15. Why do you need separate create_sql() function? Can you generate an expression
during parts initialization?
> +void
> +set_index_def(Parse *parse, Index *index, Table *table,
> + const char *name,
> + uint32_t name_len, int on_error,
> + ExprList *expr_list, u8 idx_type)
16. Big problems with indentation.
> +{
> + struct space_def *space_def = table->def;
> + uint32_t iid = SQLITE_PAGENO_TO_INDEXID(index->tnum);
17. You do not know iid here. The parser just parses and does not
generate identifiers. I checked this code in a debugger, and in this example:
box.sql.execute("CREATE TABLE t1(x integer primary key, a integer unique);")
It creates two struct Index with the same def->iid == 0. It works only thanks
to that Index.def->iid is not used anywhere now.
> + struct index_opts opts;
> + index_opts_create(&opts);
> + opts.stat = NULL;
18. Already done by index_opts_create.
> + opts.is_unique = on_error != ON_CONFLICT_ACTION_NONE;
> +
> + struct key_def *key_def = key_def_new(expr_list->nExpr);
19. Please, check each function result on error. Key_def_new can
fail.
> +
> + if (idx_type == SQLITE_IDXTYPE_APPDEF) {
> + opts.sql = create_sql(name, table->def, expr_list);
> + }
20. Please, do not put 'if' body in {} when it consists of a single line.
> +
> + int i = 0;
21. Why can not you put 'int i = 0' into 'for (...'?
> + Expr *column_expr;
> + Expr *collation_expr;
22. Same. These variables are unused out of 'for'. And can be declared +
initialized inside.
> + for (i = 0; i < expr_list->nExpr; i++) {
> + column_expr = sqlite3ExprSkipCollate(expr_list->a[i].pExpr);
> + collation_expr = expr_list->a[i].pExpr == column_expr ?
> + NULL : expr_list->a[i].pExpr;
23. Same as 11.
> +
> + uint32_t fieldno = column_expr->iColumn;
> +
> + uint32_t coll_id;
> + struct coll *coll;
> + if (collation_expr != NULL)
> + coll = sql_get_coll_seq(parse, collation_expr->u.zToken,&coll_id);
> + else
> + coll = sql_column_collation(space_def, fieldno, &coll_id);
> +
> + key_def_set_part(key_def, i, fieldno,
> + space_def->fields[fieldno].type,
> + space_def->fields[fieldno].nullable_action,
> + coll, coll_id, SORT_ORDER_ASC);
> + }
> +
> + struct key_def *pk_key_def;
> + if (idx_type == SQLITE_IDXTYPE_APPDEF) {
> + pk_key_def = table->pIndex->def->key_def;
> + } else {
> + pk_key_def = NULL;
> + }
> +
> + index->def = index_def_new(space_def->id, iid, name, name_len,
> + TREE, &opts, key_def, pk_key_def);
24. Same as 16.
> @@ -3068,6 +3200,11 @@ sql_create_index(struct Parse *parse, struct Token *token,
> */
> pIndex->sort_order[i] = SORT_ORDER_ASC;
> }
> +
> + set_index_def(parse, pIndex, pTab, zName,
> + nName, on_error, col_list,
> + idx_type);
> +
25. It fits in 2 lines.
> @@ -3133,6 +3270,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
> }
> if (idx_type == SQLITE_IDXTYPE_PRIMARYKEY)
> pIdx->idxType = idx_type;
> +
> goto exit_create_index;
> }
> }
26. Garbage diff.
> @@ -3141,6 +3279,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
> /* Link the new Index structure to its table and to the other
> * in-memory database structures.
> */
> +
> assert(parse->nErr == 0);
> if (db->init.busy) {
> Index *p;
27. Same.
28. index_column_count() is useless now.
29. index_is_unique_not_null() - same.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 5fbcbaffc..34ac846cf 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -4298,7 +4298,7 @@ sqlite3IndexedByLookup(Parse * pParse, struct SrcList_item *pFrom)
> char *zIndexedBy = pFrom->u1.zIndexedBy;
> Index *pIdx;
> for (pIdx = pTab->pIndex;
> - pIdx && strcmp(pIdx->zName, zIndexedBy);
> + pIdx && strcmp(pIdx->def->name, zIndexedBy);
> pIdx = pIdx->pNext) ;
> if (!pIdx) {
> sqlite3ErrorMsg(pParse, "no such index: %s", zIndexedBy,
30. If you use def->name instead of zName, then remove zName from struct Index.
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 950409d93..fe61dacfe 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2124,6 +2124,7 @@ struct Index {
> * or _NONE
> */
> unsigned idxType:2; /* 1==UNIQUE, 2==PRIMARY KEY, 0==CREATE INDEX */
> + struct index_def *def;
> };
31. 4 spaces + 1 tab is bad indentation. Here 1 tab is enough.
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index 496b41725..6d95185b3 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -470,7 +470,7 @@ findIndexCol(Parse * pParse, /* Parse context */
> for (int i = 0; i < pList->nExpr; i++) {
> Expr *p = sqlite3ExprSkipCollate(pList->a[i].pExpr);
> if (p->op == TK_COLUMN &&
> - p->iColumn == pIdx->aiColumn[iCol] &&
> + p->iColumn == (int) pIdx->def->key_def->parts[iCol].fieldno &&
> p->iTable == iBase) {
> bool is_found;
> uint32_t id;
32. Out of 80.
> @@ -2859,6 +2859,19 @@ whereLoopAddBtree(WhereLoopBuilder * pBuilder, /* WHERE clause information */
> sPk.aiRowLogEst = aiRowEstPk;
> sPk.onError = ON_CONFLICT_ACTION_REPLACE;
> sPk.pTable = pTab;
> +
> + struct key_def *key_def = key_def_new(1);
33. Check on error.
> + key_def_set_part(key_def, 0, 0, pTab->def->fields[0].type,
> + ON_CONFLICT_ACTION_ABORT,
> + NULL, COLL_NONE, SORT_ORDER_ASC);
> +
> + struct index_opts index_opts = index_opts_default;
> +
> + sPk.def =
> + index_def_new(pTab->def->id, 0, "primary",
> + sizeof("primary") - 1, TREE,
> + &index_opts, key_def, NULL);
34. Same as 33. And it fits in 3 lines.
> diff --git a/test/box/suite.ini b/test/box/suite.ini
> index ddc71326b..94bc39c57 100644
> --- a/test/box/suite.ini
> +++ b/test/box/suite.ini
> @@ -2,7 +2,7 @@
> core = tarantool
> description = Database tests
> script = box.lua
> -disabled = rtree_errinj.test.lua tuple_bench.test.lua
> +disabled = rtree_errinj.test.lua tuple_bench.test.lua errinj.test.lua
> release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua
> lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua
> use_unix_sockets = True
35. Why?
On 28/05/2018 11:51, Ivan Koptelov wrote:
More information about the Tarantool-patches
mailing list