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

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

Hello. Thanks for the patch, it looks almost ok, your patches become 
better and better! See my 11 comments below.


On 16/04/2018 19:35, Kirill Shcherbatov wrote:
> Introduced space_def field in SQL Table structure which
> already contains Expr field.
> 
> Needed for #3051.

1. Please, use this guide to write commit messages: 
https://tarantool.io/en/doc/1.9/dev_guide/developer_guidelines.html#how-to-write-a-commit-message

In particular this:
- limit the subject line to 50 characters,
- prefix the subject with a subsystem name. Here it is 'sql: ',
- do not end the subject line with a period,
- use the imperative mood in the subject line.

> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 22bd3ca..5a4fd6d 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
> @@ -239,6 +239,8 @@ space_def_destroy_fields(struct field_def *fields, uint32_t field_count)
>   void
>   space_def_delete(struct space_def *def)
>   {
> +	if (def == NULL)
> +		return;

2. The space_def_delete must not be called with NULL. Please, fix the
caller function instead of space_def_delete.

>   	space_opts_destroy(&def->opts);
>   	tuple_dictionary_unref(def->dict);
>   	space_def_destroy_fields(def->fields, def->field_count);
> diff --git a/src/box/sql.c b/src/box/sql.c
> index a6713f1..6418cbd 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1466,7 +1466,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>   	for (i = 0; i < n; i++) {
>   		const char *t;
>   		struct coll *coll = NULL;
> -		struct Expr *def = aCol[i].pDflt;
> +		struct field_def *field = sql_field_get(pTable, i);

3. The wrapper for assertions only is useless. Please, inline it with
no these obvious assertions.

> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 129ef82..d2e0968 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -297,7 +298,6 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
>   		Column *pCol = &pNew->aCol[i];
>   		pCol->zName = sqlite3DbStrDup(db, pCol->zName);
>   		pCol->zColl = 0;
> -		pCol->pDflt = 0;

4. I jumped around the patch and it looks, that you forgot the main part
of it - removal of struct Column.pDflt. Thanks to you, now it is very
easy.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 92f3cb6..d6033c9 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -490,6 +495,53 @@ sqlite3PrimaryKeyIndex(Table * pTab)
>   	return p;
>   }
>   
> +static Table *
> +sql_table_new(Parse *pParse, char *zName, uint32_t nFields)

5. Why do you need nFields argument? It is always 1. And I
can not understand why 1? When a parser sees 'CREATE TABLE name (', it
does not know even about a first column. Please, write a comments for
the function.

> +{
> +
> +	sqlite3 *db = pParse->db;
> +
> +
> +	Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
> +	struct space_def *def = space_def_new(0 /* space id */, 0 /* user id */,
> +					      0, "ephemeral",
> +					      strlen("ephemeral"), "memtx",
> +					      strlen("memtx"),
> +					      &space_opts_default,
> +					      &field_def_default,
> +					      0/* length of field_def */);

6. Looks like you copy-pasted this code. The space_def is created for
defaults only, and it must be mentioned here in comments. Other
space_def fields are dummy and invalid. It means that its name must be
NULL, engine must be NULL, fields array must be NULL. And do not put
comments on each argument - it must be done in function declaration, but
not on each function call. Ok? Please, fix it. I do not know, why
ephemeral tables violate these rules(
> @@ -585,6 +626,49 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
>   	return;
>   }
>   
> +/**
> + * 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)

Off topic: good name.

> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
> index f56b6d9..625cf3c 100644
> --- a/src/box/sql/fkey.c
> +++ b/src/box/sql/fkey.c
> @@ -1346,8 +1346,10 @@ fkActionTrigger(Parse * pParse,	/* Parse context */
>   									     &tToCol,
>   									     0));
>   				} else if (action == OE_SetDflt) {
> -					Expr *pDflt =
> -					    pFKey->pFrom->aCol[iFromCol].pDflt;
> +					struct field_def *field =
> +						sql_field_get(pFKey->pFrom,
> +							      iFromCol);

7. It is not DDL, so here please use space_def from the space, not from
the table. Use here space_column_default_expr.
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index b24d55b..f6db89a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1801,14 +1801,18 @@ 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)
> +			struct field_def *pSrcField = sql_field_get(pSrc, i);
> +			struct field_def *pDestField = sql_field_get(pSrc, i);

8. Same as 7.
> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index b724c98..c0bf7fd 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -359,6 +359,9 @@ sqlite3Pragma(Parse * pParse, Token * pId,	/* First part of [schema.]id field */
>   				sqlite3ViewGetColumnNames(pParse, pTab);
>   				for (i = 0, pCol = pTab->aCol; i < pTab->nCol;
>   				     i++, pCol++) {
> +					struct field_def *field =
> +						sql_field_get(pTab, i);
> +					Expr *pDflt = field->default_value_expr;

9. Same as 7.
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 59662cf..9a7d99c 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1970,6 +1970,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;

10. Please, write a comment, for what this member is used, and what
stores.


>   };
>   
>   /*
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 83c05ab..f6aa24b 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -75,7 +75,10 @@ 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 = pTab->def ?
> +			     sql_field_get(pTab, i)->default_value_expr : NULL;
> +		sqlite3ValueFromExpr(sqlite3VdbeDb(v),
> +				     expr,
>   				     pCol->affinity, &pValue);

11. Same as 7.

>   		if (pValue) {
>   			sqlite3VdbeAppendP4(v, pValue, P4_MEM);
>