[tarantool-patches] Re: [PATCH] sql: get rid off tnum field of struct Table

["n.pettik" <korablev@tarantool.org>]

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 3af9f9a..504701d 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -384,7 +384,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
> 	if (pTab == NULL)
> 		goto insert_cleanup;
> 
> -	space_id = SQLITE_PAGENO_TO_SPACEID(pTab->tnum);
> +	space_id = pTab->def->id;
> 
> 	/* Figure out if we have any triggers and if the table being
> 	 * inserted into is a view
> @@ -742,7 +742,7 @@ sqlite3Insert(Parse * pParse,	/* Parser context */
> 				if (i == pTab->iAutoIncPKey) {
> 					sqlite3VdbeAddOp2(v,
> 							  OP_NextAutoincValue,
> -							  pTab->tnum,
> +							  SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(pTab->def->id, 0),

It looks disgusting.. Lets also refactor OP_NextAutoincValue so that
it accepts raw space_id. After that, remove these SQLITE_PAGENO
macroses at all.

> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index cabe22b..0c838fa 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -432,9 +432,8 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
> 			for (i = sqliteHashFirst(&db->pSchema->tblHash); i;
> 			     i = sqliteHashNext(i)) {
> 				Table *pTab = sqliteHashData(i);
> -				uint32_t space_id =
> -					SQLITE_PAGENO_TO_SPACEID(pTab->tnum);
> -				struct space *space = space_by_id(space_id);
> +				struct space *space;
> +				space = space_by_id(pTab->def->id);

Squash two lines into one:
struct space *space = space_by_id(pTab->def->id);

> @@ -161,18 +142,18 @@ extern int
> sqlite3InitDatabase(sqlite3 * db)
> {
> 	int rc;
> -	InitData initData;
> +	struct init_data init;
> 
> 	assert(db->pSchema != NULL);
> 
> -	memset(&initData, 0, sizeof(InitData));
> -	initData.db = db;
> +	memset(&init, 0, sizeof(init));
> +	init.db = db;
> 
> 	/* Load schema from Tarantool - into the primary db only. */
> -	tarantoolSqlite3LoadSchema(&initData);
> +	tarantoolSqlite3LoadSchema(&init);
> 
> -	if (initData.rc) {
> -		rc = initData.rc;
> +	if (init.rc) {
> +		rc = init.rc;
> 		goto error_out;
> 	}
> 
> -/*
> +/**
>  * A pointer to this structure is used to communicate information
> - * from sqlite3Init and OP_ParseSchema into the sqlite3InitCallback.
> - */
> -typedef struct {
> -	sqlite3 *db;		/* The database being initialized */
> -	char **pzErrMsg;	/* Error message stored here */
> -	int rc;			/* Result code stored here */
> -} InitData;
> + * from sqlite3Init and OP_ParseSchema into the sql_init_callback.
> + */
> +struct init_data {
> +	/* The database being initialized */
> +	sqlite3 *db;
> +	/* Error message stored here */
> +	char **pzErrMsg;
> +	/* Result code stored here */

Comments inside struct definition should look like:
/** … */
And put dot at the end of sentences.

> 
> diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h
> index f043a60..b34e671 100644
> --- a/src/box/sql/tarantoolInt.h
> +++ b/src/box/sql/tarantoolInt.h
> @@ -44,7 +44,7 @@
>   ((pgno) & 1023)
> 
> /* Load database schema from Tarantool. */
> -void tarantoolSqlite3LoadSchema(InitData * init);
> +void tarantoolSqlite3LoadSchema(struct init_data * init);

struct init_data *init — remove extra space.

> @@ -2178,8 +2171,8 @@ whereLoopInsert(WhereLoopBuilder * pBuilder, WhereLoop * pTemplate)
> 	}
> 	rc = whereLoopXfer(db, p, pTemplate);
> 	Index *pIndex = p->pIndex;
> -	if (pIndex && pIndex->tnum == 0)
> -		p->pIndex = 0;
> +	if (pIndex != NULL && pIndex->pTable->def->opts.is_view)
> +		p->pIndex = NULL;
> 	return rc;
> }

These conditions don’t seem to be equivalent: view features ordinary space_id AFAIK.

> 
> @@ -2347,8 +2340,8 @@ whereRangeVectorLen(Parse * pParse,	/* Parsing context */
>  * terms only. If it is modified, this value is restored before this
>  * function returns.
>  *
> - * If pProbe->tnum==0, that means pIndex is a fake index used for the
> - * INTEGER PRIMARY KEY.
> + * If pProbe->def->space_id==0, that means pIndex is a fake index
> + * used for the INTEGER PRIMARY KEY.

But in fact you use another sign of fake index:
iid == UINT32_MAX

>  */
> static int
> whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder,	/* The WhereLoop factory */
> @@ -2391,12 +2384,11 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder,	/* The WhereLoop factory */
> 		opMask =
> 		    WO_EQ | WO_IN | WO_GT | WO_GE | WO_LT | WO_LE | WO_ISNULL;
> 	}
> -	struct space *space =
> -		space_by_id(SQLITE_PAGENO_TO_SPACEID(pProbe->tnum));
> +	struct space *space = space_by_id(pProbe->def->space_id);
> 	struct index *idx = NULL;
> 	struct index_stat *stat = NULL;
> -	if (space != NULL) {
> -		idx = space_index(space, SQLITE_PAGENO_TO_INDEXID(pProbe->tnum));
> +	if (space != NULL && pProbe->def->iid != UINT32_MAX) {
> +		idx = space_index(space, pProbe->def->iid);
> 		assert(idx != NULL);
> 		stat = idx->def->opts.stat;
> 	}
> 
> @@ -2577,7 +2569,7 @@ whereLoopAddBtreeIndex(WhereLoopBuilder * pBuilder,	/* The WhereLoop factory */
> 			assert(eOp & (WO_ISNULL | WO_EQ | WO_IN));
> 
> 			assert(pNew->nOut == saved_nOut);
> -			if (pTerm->truthProb <= 0 && pProbe->tnum != 0 ) {
> +			if (pTerm->truthProb <= 0 && !pProbe->pTable->def->opts.is_view) {

The same as previous remark: these conditions don’t seem to be equivalent