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

[n.pettik]

Hello. Several minor comments. Overall, seems OK to me.

> +/**
> + * 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 is
> + *         changed.
> + * @retval not NULL on success.
> + */
> +static Table *
> +sql_table_new(Parse *pParse, char *zName)
> +{

I would use Tarantool codestyle as well for args and vars
inside function. It is not mandatory, but would look better.

> +/**
> + * 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)
> +{

The same is here: you can use Tarantool codestyle for vars/args.
Also, I strongly recommend you to add some explanation
comments, in particular concerning allocating strategy.
It doesn’t seem to be obvious (at least for independent reviewer).

> +	sqlite3 *db = pParse->db;
> +	struct field_def *field;
> +	assert(pTable->def);

Use explicit != NULL check.

> +	assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
> +	assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);

I have grepped through code, and it seems that this 'limit'
is valid only under SQLITE_MAX_COLUMN macros.
What about removing this macros (i.e. we will make this feature be always enabled),
or surrounding your code with #ifdef directives?

> +
> +	if (id >= pTable->def->exact_field_count) {
> +		uint32_t nCol = pTable->def->exact_field_count;
> +		nCol = (nCol > 0) ? 2 * nCol : 1;
> +		field = sqlite3DbRealloc(db, pTable->def->fields,
> +					 nCol * sizeof(pTable->def->fields[0]));

What about using simple malloc/realloc? It would allow you
to get rid of pParse argument.

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index b24d55b07..067f50a0a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1801,14 +1801,21 @@ xferOptimization(Parse * pParse,	/* Parser context */
> 		}
> 		/* Default values for second and subsequent columns need to match. */
> 		if (i > 0) {
> -			assert(pDestCol->pDflt == 0
> -			       || pDestCol->pDflt->op == TK_SPAN);
> -			assert(pSrcCol->pDflt == 0
> -			       || pSrcCol->pDflt->op == TK_SPAN);
> -			if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
> -			    || (pDestCol->pDflt
> -				&& strcmp(pDestCol->pDflt->u.zToken,
> -					  pSrcCol->pDflt->u.zToken) != 0)
> +			uint32_t src_space_id =
> +				SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
> +			struct space *src_space =
> +				space_cache_find(src_space_id);

space_cache_find() can return NULL pointer. I would check it on nullability somehow.

> +			uint32_t dest_space_id =
> +				SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
> +			struct space *dest_space =
> +				space_cache_find(dest_space_id);
> +			char *src_expr_str =
> +				src_space->def->fields[i].default_value;
> +			char *dest_expr_str =
> +				dest_space->def->fields[i].default_value;
> +			if ((dest_expr_str == NULL) != (src_expr_str == NULL)
> +			    || (dest_expr_str

Codestyle nitpicking: put binary operator on previous line:
 … A &&
B