[tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.

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

Hello. Thanks for fixes! See my 11 new comments below.

> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 129ef82..39ae070 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -161,7 +161,8 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>  
>  	zTab = &pNew->zName[16];	/* Skip the "sqlite_altertab_" prefix on the name */
>  	pCol = &pNew->aCol[pNew->nCol - 1];
> -	pDflt = pCol->pDflt;
> +	assert(pNew->def);

1. Please, use explicit != NULL, when compare pointers. In other places
too.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 92f3cb6..394ee3a 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -397,6 +396,12 @@ deleteTable(sqlite3 * db, Table * pTable)
>  	sqlite3DbFree(db, pTable->zColAff);
>  	sqlite3SelectDelete(db, pTable->pSelect);
>  	sqlite3ExprListDelete(db, pTable->pCheck);
> +	if (pTable->def) {
> +		/* fields has been allocated on separate region */
> +		struct field_def *fields = pTable->def->fields;
> +		space_def_delete(pTable->def);
> +		sqlite3DbFree(db, fields);
> +	}

2. Please, do not use 'region' term here. In Tarantool region is
the allocator, and this comment slightly confuses, since the
fields array is allocated on heap instead of region.

More style comments: start a comment with capital letter and put
a dot at the end of sentences.

> @@ -490,6 +495,49 @@ sqlite3PrimaryKeyIndex(Table * pTab)
>  	return p;
>  }
>  
> +/**
> + * Create and initialize a new SQL Table object.
> + * @param pParse SQL Parser object.
> + * @param zName Table to create name.
> + * @retval NULL on memory allocation error, Parser state changed.
> + * @retval not NULL on success.
> + */
> +static Table *
> +sql_table_new(Parse *pParse, char *zName)
> +{
> +	sqlite3 *db = pParse->db;
> +
> +	Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
> +	struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
> +					      &space_opts_default,
> +					      &field_def_default, 0);
> +	if (pTable == NULL || def == NULL) {
> +		assert(db->mallocFailed);

3. This assertion fails, if space_def_new is failed, because
it does not set db flags.

> +	pTable->def = def;
> +	/* store allocated fields count */
> +	def->exact_field_count = 0;

4. Same as 2 - comments style.

> +	def->exact_field_count = 0;
> +	def->field_count = 0;
> +	pTable->def->fields = NULL;

5. These fields are initialized already in space_def_new.

> +/**
> + * Get field by id. Allocate memory if needed.
> + * @param pParse SQL Parser object.
> + * @param pTable SQL Table object.
> + * @param id column identifier.
> + * @retval not NULL on success.
> + * @retval NULL on out of memory.
> + */
> +static struct field_def *
> +sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
> +{
> +	sqlite3 *db = pParse->db;
> +	struct field_def *field;
> +	assert(pTable->def);
> +	assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
> +	assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
> +
> +	if (id >= pTable->def->exact_field_count) {
> +		uint32_t nCol = pTable->def->exact_field_count;
> +		nCol = (nCol > 0) ? 2*nCol : 1;

6. Please, put spaces around arithmetic operators.

> +		for (uint32_t i = nCol/2; i < nCol; i++)
> +		     memcpy(&field[i], &field_def_default,
> +			    sizeof(struct field_def));

7. Same as 6. And put a 'for' body into {}, when it consists of
multiple lines.

> @@ -813,7 +894,11 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
>  			 * is required by pragma table_info.
>  			 */
>  			Expr x;
> -			sql_expr_free(db, pCol->pDflt, false);
> +			assert(p->def);

8. Same as 1.

> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
> index f56b6d9..d196fd4 100644
> --- a/src/box/sql/fkey.c
> +++ b/src/box/sql/fkey.c
> @@ -36,6 +36,8 @@
>  #include <box/coll.h>
>  #include "sqliteInt.h"
>  #include "box/session.h"
> +#include "box/schema.h"

9. Unused header.

> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 59662cf..90b7e08 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1970,6 +1969,7 @@ struct Table {
>  	Trigger *pTrigger;	/* List of triggers stored in pSchema */
>  	Schema *pSchema;	/* Schema that contains this table */
>  	Table *pNextZombie;	/* Next on the Parse.pZombieTab list */
> +	struct space_def *def;	/* Space definition with Tarantool metadata */

10. For new members please use Tarantool code style - the
comment above the member.

> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 83c05ab..8d1cfdd 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -35,6 +35,8 @@
>   */
>  #include "sqliteInt.h"
>  #include "box/session.h"
> +#include "tarantoolInt.h"
> +#include "box/schema.h"
>  
>  /*
>   * The most recently coded instruction was an OP_Column to retrieve the
> @@ -75,7 +77,18 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, int iReg)
>  		Column *pCol = &pTab->aCol[i];
>  		VdbeComment((v, "%s.%s", pTab->zName, pCol->zName));
>  		assert(i < pTab->nCol);
> -		sqlite3ValueFromExpr(sqlite3VdbeDb(v), pCol->pDflt,
> +
> +		Expr *expr = NULL;
> +		struct space *space =
> +			space_cache_find(SQLITE_PAGENO_TO_SPACEID(pTab->tnum));
> +		/* FIXME: ephemeral spaces are not present in the cache now */
> +		if (space != NULL && space->def->fields != NULL)
> +			expr = space->def->fields[i].default_value_expr;
> +		if (expr == NULL && pTab->def != NULL)
> +			expr = pTab->def->fields[i].default_value_expr;
> +

11. You must not use default from pTab->def after DDL is finished. As I
remember we discussed it verbally, so lets just remove it.