[tarantool-patches] [PATCH] sql: add index_def to struct Index

[tarantool-patches] [PATCH] sql: add index_def to struct Index

[Ivan Koptelov]

Currently there is a problem with
non-deterministic nullability in
index_def. To see the error just
uncomment printfs on build.c 2999
and run any sql-tap test that
has to do with indexes, e.g
distinct.test.lua.
github branch: https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where
---
 src/box/sql/build.c     | 179 +++++++++++++++++++++++++++++++++++++++++++++++-
 src/box/sql/sqliteInt.h |   2 +
 2 files changed, 179 insertions(+), 2 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 742f8a572..6bd70fa9f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -43,6 +43,7 @@
  *     COMMIT
  *     ROLLBACK
  */
+#include <ctype.h>
 #include "sqliteInt.h"
 #include "vdbeInt.h"
 #include "tarantoolInt.h"
@@ -2841,9 +2842,173 @@ index_is_unique(Index *idx)
 	return tnt_index->def->opts.is_unique;
 }
 
+/*
+ * In CREATE INDEX requests user may set binary collation with COLLATE
+ * [BINARY, binary, "binary" and "bInarY"] This function is to handle all
+ * these cases.
+ */
+bool is_binary_collation(const char *coll_name)
+{
+	char *upper_case_name =
+		malloc(sizeof(*coll_name) * strlen(coll_name));
+	for (int i = 0; i < (int) strlen(coll_name); i++) {
+		upper_case_name[i] = toupper(coll_name[i]);
+	}
+
+	bool res = (strcmp(upper_case_name, "BINARY") == 0);
+	free(upper_case_name);
+	return res;
+}
+
+//bool is_index_equal(Index * old, struct index_def *new)
+//{
+//	assert(strcmp(old->zName, new->name);
+//	return true;
+//}
+
+struct index_def *create_index_def(Table *pTab, const char *name,
+				   uint32_t name_len, int onError,
+				   ExprList *pList, u8 idxType)
+{
+	struct space_def *space_def = pTab->def;
+	struct space *space = space_by_id(space_def->id);
+
+	uint32_t iid = 0;
+	Index *temp = pTab->pIndex;
+	while (temp != NULL) {
+		iid++;
+		temp = temp->pNext;
+	}
+
+	struct index_opts *opts = (struct index_opts *) malloc(sizeof(*opts));
+	index_opts_create(opts);
+	opts->sql = NULL;
+
+	if (onError == ON_CONFLICT_ACTION_NONE) {
+		opts->is_unique = false;
+	} else {
+		opts->is_unique = true;
+	}
+
+	struct key_part_def *part_def_tmp = (struct key_part_def *)
+		malloc(sizeof(*part_def_tmp) * pList->nExpr);
+
+	uint32_t part_count = 0;
+	int i = 0;
+	struct ExprList_item *pListItem;
+
+	/*
+	 * In case of ordinary index creation we want ot store a
+	 * SQL statement of this creation.
+	 */
+	if (idxType == SQLITE_IDXTYPE_APPDEF) {
+		asprintf(&opts->sql,"CREATE INDEX %s ON %s(",
+			 name, space_def->name);
+	}
+
+	for (i = 0, pListItem = pList->a; i < pList->nExpr; pListItem++, i++) {
+		Expr *pCExpr = sqlite3ExprSkipCollate(pListItem->pExpr);
+
+		if (pCExpr->op == TK_COLUMN) {
+			uint32_t fieldno = pCExpr->iColumn;
+			part_def_tmp[i].fieldno = fieldno;
+			part_def_tmp[i].nullable_action =
+				ON_CONFLICT_ACTION_DEFAULT;
+			part_def_tmp[i].type =
+				space_def->fields[fieldno].type;
+
+//			part_def_tmp[i].is_nullable =
+//				(fieldno < space_def->field_count ?
+//				 space_def->fields[fieldno].is_nullable :
+//				 false);
+
+			part_def_tmp[i].is_nullable =
+				table_column_is_nullable(pTab->def, fieldno);
+
+			part_def_tmp[i].coll_id =
+				space_def->fields[fieldno].coll_id ?
+				space_def->fields[fieldno].coll_id : COLL_NONE;
+			part_count++;
+
+			if (opts->sql != NULL) {
+				if (part_count == 1) {
+					asprintf(&opts->sql,"%s%s,",opts->sql,
+						 space_def->fields[fieldno].name);
+				} else {
+					asprintf(&opts->sql,"%s %s,",opts->sql,
+						 space_def->fields[fieldno].name);
+				}
+			}
+		} else {
+			assert(pCExpr->op == TK_COLUMN);
+		}
+
+		if (pListItem->pExpr->op == TK_COLLATE) {
+			const char *coll_name = pListItem->pExpr->u.zToken;
+			assert(coll_name != NULL);
+			size_t coll_name_len = strlen(coll_name);
+			struct coll *coll = coll_by_name(coll_name, coll_name_len);
+
+			if (is_binary_collation(coll_name)) {
+				part_def_tmp[i].coll_id = COLL_NONE;
+			} else {
+				assert(coll != NULL);
+				part_def_tmp[i].coll_id = coll->id;
+			}
+
+			if (opts->sql != NULL) {
+				/*
+				 * If index part has a collation then
+				 * corresponding part of statement should
+				 * be changed from 'column_name,' to
+				 * 'column_name COLLATE collation_name,'.
+				 */
+				opts->sql[strlen(opts->sql) - 1] = ' ';
+				asprintf(&opts->sql,"%sCOLLATE %s,", opts->sql,
+					 coll_name);
+			}
+		}
+		if (pListItem->pExpr->op != TK_COLUMN &&
+		    pListItem->pExpr->op != TK_COLLATE) {
+			printf("error %d\n", pListItem->pExpr->op);
+		}
+	}
+
+	if (idxType == SQLITE_IDXTYPE_APPDEF) {
+		opts->sql[strlen(opts->sql) - 1] = ')';
+	}
+
+	struct key_part_def *part_def = (struct key_part_def *)
+		malloc(sizeof(*part_def) * part_count + 1);
+
+	memcpy(part_def, part_def_tmp, sizeof(*part_def) * part_count);
+	free(part_def_tmp);
+
+	struct key_def *key_def = key_def_new_with_parts(part_def, part_count);
+	assert(key_def != NULL);
+
+	struct key_def *pk_key_def;
+	if (idxType == SQLITE_IDXTYPE_APPDEF) {
+		pk_key_def = space_index_key_def(space, 0);
+	} else {
+		pk_key_def = NULL;
+	}
+
+	enum index_type default_type = TREE;
+
+	printf("space id %d index name %s index iid %d\n", space_def->id, name, iid);
+	printf("nullables\n");
+	for (uint32_t j = 0; j < key_def->part_count; j++){
+		printf("%d\n", key_part_is_nullable(&key_def->parts[i]));
+	}
+
+	return index_def_new(space_def->id, iid, name, name_len,
+			     default_type, opts, key_def, pk_key_def);
+}
+
 /*
  * Create a new index for an SQL table.  pName1.pName2 is the name of the index
- * and pTblList is the name of the table that is to be indexed.  Both will
+ * and pTblName is the name of the table that is to be indexed.  Both will
  * be NULL for a primary key or an index that is created to satisfy a
  * UNIQUE constraint.  If pTable and pIndex are NULL, use pParse->pNewTable
  * as the table to be indexed.  pParse->pNewTable is a table that is
@@ -3187,6 +3352,13 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
 	/* Link the new Index structure to its table and to the other
 	 * in-memory database structures.
 	 */
+
+	if (pIndex->def == NULL) {
+		pIndex->def =
+			create_index_def(pTab, zName, nName, onError,
+					 pList, idxType);
+	}
+
 	assert(pParse->nErr == 0);
 	if (db->init.busy) {
 		Index *p;
@@ -3283,8 +3455,11 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
 
 	/* Clean up before exiting */
  exit_create_index:
-	if (pIndex)
+	if (pIndex) {
+		if (pIndex->def)
+			index_def_delete(pIndex->def);
 		freeIndex(db, pIndex);
+	}
 	sql_expr_free(db, pPIWhere, false);
 	sqlite3ExprListDelete(db, pList);
 	sqlite3SrcListDelete(db, pTblName);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 4c2c6fb69..b24abcf2b 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2151,6 +2151,8 @@ struct Index {
 	IndexSample *aSample;	/* Samples of the left-most key */
 	tRowcnt *aiRowEst;	/* Non-logarithmic stat1 data for this index */
 	tRowcnt nRowEst0;	/* Non-logarithmic number of rows in the index */
+	/** Index definition with Tarantool metadata. */
+	struct index_def *def;
 };
 
 /*
-- 
2.14.3 (Apple Git-98)

[tarantool-patches] Re: [PATCH] sql: add index_def to struct Index

[Kirill Yukhin]

On 22 мая 19:03, Ivan Koptelov wrote:
> Currently there is a problem with
> non-deterministic nullability in
> index_def. To see the error just
> uncomment printfs on build.c 2999
> and run any sql-tap test that
> has to do with indexes, e.g
> distinct.test.lua.
> github branch: https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where
> ---
>  src/box/sql/build.c     | 179 +++++++++++++++++++++++++++++++++++++++++++++++-
>  src/box/sql/sqliteInt.h |   2 +
>  2 files changed, 179 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 742f8a572..6bd70fa9f 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -43,6 +43,7 @@
>   *     COMMIT
>   *     ROLLBACK
>   */
> +#include <ctype.h>
>  #include "sqliteInt.h"
>  #include "vdbeInt.h"
>  #include "tarantoolInt.h"
> @@ -2841,9 +2842,173 @@ index_is_unique(Index *idx)
>  	return tnt_index->def->opts.is_unique;
>  }
>  
> +/*
> + * In CREATE INDEX requests user may set binary collation with COLLATE
> + * [BINARY, binary, "binary" and "bInarY"] This function is to handle all
> + * these cases.
> + */
> +bool is_binary_collation(const char *coll_name)
> +{
> +	char *upper_case_name =
> +		malloc(sizeof(*coll_name) * strlen(coll_name));
> +	for (int i = 0; i < (int) strlen(coll_name); i++) {
> +		upper_case_name[i] = toupper(coll_name[i]);
> +	}
> +
> +	bool res = (strcmp(upper_case_name, "BINARY") == 0);
> +	free(upper_case_name);
> +	return res;
> +}
> +
> +//bool is_index_equal(Index * old, struct index_def *new)
> +//{
> +//	assert(strcmp(old->zName, new->name);
> +//	return true;
> +//}
Junk.
> +
> +//			part_def_tmp[i].is_nullable =
> +//				(fieldno < space_def->field_count ?
> +//				 space_def->fields[fieldno].is_nullable :
> +//				 false);
Junk.
> @@ -3283,8 +3455,11 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
>  
>  	/* Clean up before exiting */
>   exit_create_index:
> -	if (pIndex)
> +	if (pIndex) {
> +		if (pIndex->def)
!= NULL.
> +			index_def_delete(pIndex->def);
>  		freeIndex(db, pIndex);
> +	}
>  	sql_expr_free(db, pPIWhere, false);
>  	sqlite3ExprListDelete(db, pList);
>  	sqlite3SrcListDelete(db, pTblName);

[tarantool-patches] Re: [PATCH] sql: add index_def to struct Index

[Vladislav Shpilevoy]

Hello. Thanks for the patch! See 26 comments below.

On 22/05/2018 19:03, Ivan Koptelov wrote:
> Currently there is a problem with
> non-deterministic nullability in
> index_def. To see the error just
> uncomment printfs on build.c 2999
> and run any sql-tap test that
> has to do with indexes, e.g
> distinct.test.lua.
> github branch: https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where
> ---
>   src/box/sql/build.c     | 179 +++++++++++++++++++++++++++++++++++++++++++++++-
>   src/box/sql/sqliteInt.h |   2 +
>   2 files changed, 179 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 742f8a572..6bd70fa9f 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -43,6 +43,7 @@
>    *     COMMIT
>    *     ROLLBACK
>    */
> +#include <ctype.h>

1. See the comment 2 and remove it, please.

>   #include "sqliteInt.h"
>   #include "vdbeInt.h"
>   #include "tarantoolInt.h"
> @@ -2841,9 +2842,173 @@ index_is_unique(Index *idx)
>   	return tnt_index->def->opts.is_unique;
>   }
>   
> +/*
> + * In CREATE INDEX requests user may set binary collation with COLLATE
> + * [BINARY, binary, "binary" and "bInarY"] This function is to handle all
> + * these cases.
> + */
> +bool is_binary_collation(const char *coll_name)
> +{
> +	char *upper_case_name =
> +		malloc(sizeof(*coll_name) * strlen(coll_name));
> +	for (int i = 0; i < (int) strlen(coll_name); i++) {
> +		upper_case_name[i] = toupper(coll_name[i]);
> +	}
> +
> +	bool res = (strcmp(upper_case_name, "BINARY") == 0);
> +	free(upper_case_name);
> +	return res;

2. All this function is identical to simple strcasecmp(coll_name, "binary").

> +}
> +
> +//bool is_index_equal(Index * old, struct index_def *new)
> +//{
> +//	assert(strcmp(old->zName, new->name);
> +//	return true;
> +//}

3. Garbage diff.

> +
> +struct index_def *create_index_def(Table *pTab, const char *name,
> +				   uint32_t name_len, int onError,
> +				   ExprList *pList, u8 idxType)

4. According to Tarantool code style you should wrap line after return type
definition. And please, use snake_case for variable names.

> +{
> +	struct space_def *space_def = pTab->def;
> +	struct space *space = space_by_id(space_def->id);

5. space_by_id takes a space from the space cache, but during
'CREATE TABLE' generation the space is not in the cache. This will
return NULL.

You must not use space * here at all. I see, that you use it
in a single place to find a primary index key_def, but you can get it
from the pTab from the first pIndex - it is always primary one.

> +
> +	uint32_t iid = 0;
> +	Index *temp = pTab->pIndex;
> +	while (temp != NULL) {
> +		iid++;
> +		temp = temp->pNext;

6. Bad idea. You must take Index * as the function argument, and initialize its
index_def.

> +	}
> +
> +	struct index_opts *opts = (struct index_opts *) malloc(sizeof(*opts));

7. Why can not you declare it on the stack?

> +	index_opts_create(opts);
> +	opts->sql = NULL;
> +
> +	if (onError == ON_CONFLICT_ACTION_NONE) {
> +		opts->is_unique = false;
> +	} else {
> +		opts->is_unique = true;
> +	}

8. Do not put 'if' body in {} when it consists of a single line.

9. Here you can simply do this:
opts->is_unique = onError != ON_CONFLICT_ACTION_NONE;

> +
> +	struct key_part_def *part_def_tmp = (struct key_part_def *)
> +		malloc(sizeof(*part_def_tmp) * pList->nExpr);

10. Please, do not do this. Use key_def_new + multiple key_def_set_part.
You use too many mallocs.

> +
> +	uint32_t part_count = 0;
> +	int i = 0;
> +	struct ExprList_item *pListItem;
> +
> +	/*
> +	 * In case of ordinary index creation we want ot store a
> +	 * SQL statement of this creation.
> +	 */
> +	if (idxType == SQLITE_IDXTYPE_APPDEF) {
> +		asprintf(&opts->sql,"CREATE INDEX %s ON %s(",
> +			 name, space_def->name);

11. You should check if asprintf is failed. Or do not use asprintf at
all. See below how.

> +	}
> +
> +	for (i = 0, pListItem = pList->a; i < pList->nExpr; pListItem++, i++) {
> +		Expr *pCExpr = sqlite3ExprSkipCollate(pListItem->pExpr);
> +
> +		if (pCExpr->op == TK_COLUMN) {
> +			uint32_t fieldno = pCExpr->iColumn;
> +			part_def_tmp[i].fieldno = fieldno;
> +			part_def_tmp[i].nullable_action =
> +				ON_CONFLICT_ACTION_DEFAULT;

12. Wrong - action default is internal thing, that can not be
set in key_part actually. You must set action to the same value as
the space field is set.

> +			part_def_tmp[i].type =
> +				space_def->fields[fieldno].type;
> +
> +//			part_def_tmp[i].is_nullable =
> +//				(fieldno < space_def->field_count ?
> +//				 space_def->fields[fieldno].is_nullable :
> +//				 false);
> +
> +			part_def_tmp[i].is_nullable =
> +				table_column_is_nullable(pTab->def, fieldno);

13. Please, do not use table_column_is_nullable. It does space_by_id(def->id),
but here def->id is not defined when you create a primary index inside a CREATE
TABLE request.

> +
> +			part_def_tmp[i].coll_id =
> +				space_def->fields[fieldno].coll_id ?
> +				space_def->fields[fieldno].coll_id : COLL_NONE;

14. Do not use integers as booleans.

15. Why not just part_def_tmp[i].coll_id = space_def->fields[fieldno].coll_id?
Besides, coll_id == 0 is valid collation identifier. It is not the same as COLL_NONE.

> +			part_count++;

16. Why do you need this counter? You already know part count - it is equal
to pList->nExpr.

> +
> +			if (opts->sql != NULL) {

17. How can it be NULL?

> +				if (part_count == 1) {
> +					asprintf(&opts->sql,"%s%s,",opts->sql,
> +						 space_def->fields[fieldno].name);
> +				} else {
> +					asprintf(&opts->sql,"%s %s,",opts->sql,
> +						 space_def->fields[fieldno].name);
> +				}
> +			}
> +		} else {
> +			assert(pCExpr->op == TK_COLUMN);

18. So I see this:
if (something) {
  ...
} else {
  assert(something);
}

Very strange. This assert fails always.

> +		}
> +
> +		if (pListItem->pExpr->op == TK_COLLATE) {

19. Here you completely skipped column initialization when it has
COLLATE. See sqlite3ExprSkipCollate, the same code on line 3235, and
how COLLATE is stored in Expr tree (it replaces the original Expr making
it to be COLLATE's chils). This is why here pList->nExpr is the same as
part count.

> +			const char *coll_name = pListItem->pExpr->u.zToken;
> +			assert(coll_name != NULL);
> +			size_t coll_name_len = strlen(coll_name);
> +			struct coll *coll = coll_by_name(coll_name, coll_name_len);
> +
> +			if (is_binary_collation(coll_name)) {
> +				part_def_tmp[i].coll_id = COLL_NONE;
> +			} else {
> +				assert(coll != NULL);

20. Why? Collation can be not found. Use sqlite3GetCollSeq. But before do
rebase on the latest 2.0. You branch looks very out of date. Many things
were changed after the latest 1.10 into 2.0 merge.

> +				part_def_tmp[i].coll_id = coll->id;
> +			}
> +
> +			if (opts->sql != NULL) {

21. How can it be NULL?

> +				/*
> +				 * If index part has a collation then
> +				 * corresponding part of statement should
> +				 * be changed from 'column_name,' to
> +				 * 'column_name COLLATE collation_name,'.
> +				 */
> +				opts->sql[strlen(opts->sql) - 1] = ' ';
> +				asprintf(&opts->sql,"%sCOLLATE %s,", opts->sql,
> +					 coll_name);

22. Here and in all others asprintf you have leak - you reset opts->sql with new
malloc (called in asprintf), but do not free the original. And please, use region
instead. You can accumulate SQL parts via multiple region_alloc + single
region_join + strdup at the end. It makes leaks be impossible, and does not
require any malloc + free.

> +			}
> +		}
> +		if (pListItem->pExpr->op != TK_COLUMN &&
> +		    pListItem->pExpr->op != TK_COLLATE) {
> +			printf("error %d\n", pListItem->pExpr->op);
> +		}
> +	}
> +
> +	if (idxType == SQLITE_IDXTYPE_APPDEF) {
> +		opts->sql[strlen(opts->sql) - 1] = ')';
> +	}
> +
> +	struct key_part_def *part_def = (struct key_part_def *)
> +		malloc(sizeof(*part_def) * part_count + 1);
> +
> +	memcpy(part_def, part_def_tmp, sizeof(*part_def) * part_count);
> +	free(part_def_tmp);
> +
> +	struct key_def *key_def = key_def_new_with_parts(part_def, part_count);
> +	assert(key_def != NULL);

23. All this things can be removed by usage of key_def_new +
multiple key_def_set_part.

> +
> +	struct key_def *pk_key_def;
> +	if (idxType == SQLITE_IDXTYPE_APPDEF) {
> +		pk_key_def = space_index_key_def(space, 0);

24. You must not use a space here.

> +	} else {
> +		pk_key_def = NULL;
> +	}
> +
> +	enum index_type default_type = TREE;
> +
> +	printf("space id %d index name %s index iid %d\n", space_def->id, name, iid);
> +	printf("nullables\n");
> +	for (uint32_t j = 0; j < key_def->part_count; j++){
> +		printf("%d\n", key_part_is_nullable(&key_def->parts[i]));

25. You asked me why you see unstable nullability. Here the answer. You iterate over
j but use i as an index for key_def->parts array.

> +	}
> +
> +	return index_def_new(space_def->id, iid, name, name_len,
> +			     default_type, opts, key_def, pk_key_def);

26. Why no just inline TREE instead of declaring a variable default_type?

[tarantool-patches] Re: [PATCH] sql: add index_def to struct Index

[Ivan Koptelov]

[-- Attachment #1.1: Type: text/plain, Size: 9146 bytes --]




>Вторник, 22 мая 2018, 22:10 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
>
>Hello. Thanks for the patch! See 26 comments below.
>
>On 22/05/2018 19:03, Ivan Koptelov wrote:
>> Currently there is a problem with
>> non-deterministic nullability in
>> index_def. To see the error just
>> uncomment printfs on build.c 2999
>> and run any sql-tap test that
>> has to do with indexes, e.g
>> distinct.test.lua.
>> github branch:  https://github.com/tarantool/tarantool/tree/sb/gh-3369-use-index-def-in-select-and-where
>> ---
>>   src/box/sql/build.c     | 179 +++++++++++++++++++++++++++++++++++++++++++++++-
>>   src/box/sql/sqliteInt.h |   2 +
>>   2 files changed, 179 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 742f8a572..6bd70fa9f 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -43,6 +43,7 @@
>>    *     COMMIT
>>    *     ROLLBACK
>>    */
>> +#include <ctype.h>
>
>1. See the comment 2 and remove it, please.
>
>>   #include "sqliteInt.h"
>>   #include "vdbeInt.h"
>>   #include "tarantoolInt.h"
>> @@ -2841,9 +2842,173 @@ index_is_unique(Index *idx)
>>   	return tnt_index->def->opts.is_unique;
>>   }
>> 
>> +/*
>> + * In CREATE INDEX requests user may set binary collation with COLLATE
>> + * [BINARY, binary, "binary" and "bInarY"] This function is to handle all
>> + * these cases.
>> + */
>> +bool is_binary_collation(const char *coll_name)
>> +{
>> +	char *upper_case_name =
>> +		malloc(sizeof(*coll_name) * strlen(coll_name));
>> +	for (int i = 0; i < (int) strlen(coll_name); i++) {
>> +		upper_case_name[i] = toupper(coll_name[i]);
>> +	}
>> +
>> +	bool res = (strcmp(upper_case_name, "BINARY") == 0);
>> +	free(upper_case_name);
>> +	return res;
>
>2. All this function is identical to simple strcasecmp(coll_name, "binary").
>
>> +}
>> +
>> +//bool is_index_equal(Index * old, struct index_def *new)
>> +//{
>> +//	assert(strcmp(old->zName, new->name);
>> +//	return true;
>> +//}
>
>3. Garbage diff.
>
>> +
>> +struct index_def *create_index_def(Table *pTab, const char *name,
>> +				   uint32_t name_len, int onError,
>> +				   ExprList *pList, u8 idxType)
>
>4. According to Tarantool code style you should wrap line after return type
>definition. And please, use snake_case for variable names.
>
>> +{
>> +	struct space_def *space_def = pTab->def;
>> +	struct space *space = space_by_id(space_def->id);
>
>5. space_by_id takes a space from the space cache, but during
>'CREATE TABLE' generation the space is not in the cache. This will
>return NULL.
>
>You must not use space * here at all. I see, that you use it
>in a single place to find a primary index key_def, but you can get it
>from the pTab from the first pIndex - it is always primary one.
>
>> +
>> +	uint32_t iid = 0;
>> +	Index *temp = pTab->pIndex;
>> +	while (temp != NULL) {
>> +		iid++;
>> +		temp = temp->pNext;
>
>6. Bad idea. You must take Index * as the function argument, and initialize its
>index_def.
>
>> +	}
>> +
>> +	struct index_opts *opts = (struct index_opts *) malloc(sizeof(*opts));
>
>7. Why can not you declare it on the stack?
>
>> +	index_opts_create(opts);
>> +	opts->sql = NULL;
>> +
>> +	if (onError == ON_CONFLICT_ACTION_NONE) {
>> +		opts->is_unique = false;
>> +	} else {
>> +		opts->is_unique = true;
>> +	}
>
>8. Do not put 'if' body in {} when it consists of a single line.
>
>9. Here you can simply do this:
>opts->is_unique = onError != ON_CONFLICT_ACTION_NONE;
>
>> +
>> +	struct key_part_def *part_def_tmp = (struct key_part_def *)
>> +		malloc(sizeof(*part_def_tmp) * pList->nExpr);
>
>10. Please, do not do this. Use key_def_new + multiple key_def_set_part.
>You use too many mallocs.
>
>> +
>> +	uint32_t part_count = 0;
>> +	int i = 0;
>> +	struct ExprList_item *pListItem;
>> +
>> +	/*
>> +	 * In case of ordinary index creation we want ot store a
>> +	 * SQL statement of this creation.
>> +	 */
>> +	if (idxType == SQLITE_IDXTYPE_APPDEF) {
>> +		asprintf(&opts->sql,"CREATE INDEX %s ON %s(",
>> +			 name, space_def->name);
>
>11. You should check if asprintf is failed. Or do not use asprintf at
>all. See below how.
>
>> +	}
>> +
>> +	for (i = 0, pListItem = pList->a; i < pList->nExpr; pListItem++, i++) {
>> +		Expr *pCExpr = sqlite3ExprSkipCollate(pListItem->pExpr);
>> +
>> +		if (pCExpr->op == TK_COLUMN) {
>> +			uint32_t fieldno = pCExpr->iColumn;
>> +			part_def_tmp[i].fieldno = fieldno;
>> +			part_def_tmp[i].nullable_action =
>> +				ON_CONFLICT_ACTION_DEFAULT;
>
>12. Wrong - action default is internal thing, that can not be
>set in key_part actually. You must set action to the same value as
>the space field is set.
>
>> +			part_def_tmp[i].type =
>> +				space_def->fields[fieldno].type;
>> +
>> +//			part_def_tmp[i].is_nullable =
>> +//				(fieldno < space_def->field_count ?
>> +//				 space_def->fields[fieldno].is_nullable :
>> +//				 false);
>> +
>> +			part_def_tmp[i].is_nullable =
>> +				table_column_is_nullable(pTab->def, fieldno);
>
>13. Please, do not use table_column_is_nullable. It does space_by_id(def->id),
>but here def->id is not defined when you create a primary index inside a CREATE
>TABLE request.
>
>> +
>> +			part_def_tmp[i].coll_id =
>> +				space_def->fields[fieldno].coll_id ?
>> +				space_def->fields[fieldno].coll_id : COLL_NONE;
>
>14. Do not use integers as booleans.
>
>15. Why not just part_def_tmp[i].coll_id = space_def->fields[fieldno].coll_id?
>Besides, coll_id == 0 is valid collation identifier. It is not the same as COLL_NONE.
>
>> +			part_count++;
>
>16. Why do you need this counter? You already know part count - it is equal
>to pList->nExpr.
>
>> +
>> +			if (opts->sql != NULL) {
>
>17. How can it be NULL?
>
>> +				if (part_count == 1) {
>> +					asprintf(&opts->sql,"%s%s,",opts->sql,
>> +						 space_def->fields[fieldno].name);
>> +				} else {
>> +					asprintf(&opts->sql,"%s %s,",opts->sql,
>> +						 space_def->fields[fieldno].name);
>> +				}
>> +			}
>> +		} else {
>> +			assert(pCExpr->op == TK_COLUMN);
>
>18. So I see this:
>if (something) {
>  ...
>} else {
>  assert(something);
>}
>
>Very strange. This assert fails always.
>
>> +		}
>> +
>> +		if (pListItem->pExpr->op == TK_COLLATE) {
>
>19. Here you completely skipped column initialization when it has
>COLLATE. See sqlite3ExprSkipCollate, the same code on line 3235, and
>how COLLATE is stored in Expr tree (it replaces the original Expr making
>it to be COLLATE's chils). This is why here pList->nExpr is the same as
>part count.
>
>> +			const char *coll_name = pListItem->pExpr->u.zToken;
>> +			assert(coll_name != NULL);
>> +			size_t coll_name_len = strlen(coll_name);
>> +			struct coll *coll = coll_by_name(coll_name, coll_name_len);
>> +
>> +			if (is_binary_collation(coll_name)) {
>> +				part_def_tmp[i].coll_id = COLL_NONE;
>> +			} else {
>> +				assert(coll != NULL);
>
>20. Why? Collation can be not found. Use sqlite3GetCollSeq. But before do
>rebase on the latest 2.0. You branch looks very out of date. Many things
>were changed after the latest 1.10 into 2.0 merge.
>
>> +				part_def_tmp[i].coll_id = coll->id;
>> +			}
>> +
>> +			if (opts->sql != NULL) {
>
>21. How can it be NULL?
>
>> +				/*
>> +				 * If index part has a collation then
>> +				 * corresponding part of statement should
>> +				 * be changed from 'column_name,' to
>> +				 * 'column_name COLLATE collation_name,'.
>> +				 */
>> +				opts->sql[strlen(opts->sql) - 1] = ' ';
>> +				asprintf(&opts->sql,"%sCOLLATE %s,", opts->sql,
>> +					 coll_name);
>
>22. Here and in all others asprintf you have leak - you reset opts->sql with new
>malloc (called in asprintf), but do not free the original. And please, use region
>instead. You can accumulate SQL parts via multiple region_alloc + single
>region_join + strdup at the end. It makes leaks be impossible, and does not
>require any malloc + free.
>
>> +			}
>> +		}
>> +		if (pListItem->pExpr->op != TK_COLUMN &&
>> +		    pListItem->pExpr->op != TK_COLLATE) {
>> +			printf("error %d\n", pListItem->pExpr->op);
>> +		}
>> +	}
>> +
>> +	if (idxType == SQLITE_IDXTYPE_APPDEF) {
>> +		opts->sql[strlen(opts->sql) - 1] = ')';
>> +	}
>> +
>> +	struct key_part_def *part_def = (struct key_part_def *)
>> +		malloc(sizeof(*part_def) * part_count + 1);
>> +
>> +	memcpy(part_def, part_def_tmp, sizeof(*part_def) * part_count);
>> +	free(part_def_tmp);
>> +
>> +	struct key_def *key_def = key_def_new_with_parts(part_def, part_count);
>> +	assert(key_def != NULL);
>
>23. All this things can be removed by usage of key_def_new +
>multiple key_def_set_part.
>
>> +
>> +	struct key_def *pk_key_def;
>> +	if (idxType == SQLITE_IDXTYPE_APPDEF) {
>> +		pk_key_def = space_index_key_def(space, 0);
>
>24. You must not use a space here.
>
>> +	} else {
>> +		pk_key_def = NULL;
>> +	}
>> +
>> +	enum index_type default_type = TREE;
>> +
>> +	printf("space id %d index name %s index iid %d\n", space_def->id, name, iid);
>> +	printf("nullables\n");
>> +	for (uint32_t j = 0; j < key_def->part_count; j++){
>> +		printf("%d\n", key_part_is_nullable(&key_def->parts[i]));
>
Fixed all of the comments' issues.

-- 
Ivan Koptelov

[-- Attachment #1.2: Type: text/html, Size: 11473 bytes --]

[-- Attachment #2: 0001-add-index_def-to-Index.patch --]
[-- Type: application/x-patch, Size: 6096 bytes --]