[tarantool-patches] Re: [PATCH v3 2/4] sql: Remove zName and nColumn from SQL.

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

Hello. See my comment about the smaller version of this commit.
I can not see the new version on the branch, so I am reviewing
this one.

See my 8 comments below about the first part of the patch. Next part
I will review, when you push actual version.

On 25/04/2018 19:52, Kirill Shcherbatov wrote:
> 1. Removed zName from SQL Column.
> 2. Removed zColumns from SQL Table.
> 3. Refactored Parser to use def_expression directly.
> 4. Introduced sql_table_def_rebuild intended for collect
> fragmented with sql_field_retrieve space_def into memory
1. Collect fragmented what, field_def? If so, the it would be good
to write it explicitly.

> located in one allocation.
> 
> Needed for #3272.
> ---
>   src/box/space_def.c     |  29 ++++----
>   src/box/sql.c           |  61 +++++++++++++---
>   src/box/sql.h           |  23 ++++++
>   src/box/sql/alter.c     |  32 ++++++---
>   src/box/sql/analyze.c   |   5 +-
>   src/box/sql/build.c     | 181 ++++++++++++++++++++++--------------------------
>   src/box/sql/delete.c    |   6 +-
>   src/box/sql/expr.c      |  11 +--
>   src/box/sql/fkey.c      |  20 +++---
>   src/box/sql/insert.c    |  55 ++++++++-------
>   src/box/sql/pragma.c    |  24 ++++---
>   src/box/sql/resolve.c   |  16 +++--
>   src/box/sql/select.c    |  92 ++++++++++++------------
>   src/box/sql/sqliteInt.h |   4 +-
>   src/box/sql/update.c    |  29 ++++----
>   src/box/sql/where.c     |   6 +-
>   src/box/sql/wherecode.c |   2 +-
>   src/box/sql/whereexpr.c |   4 +-
>   18 files changed, 338 insertions(+), 262 deletions(-)
> 
> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 22bd3ca..77c0e02 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
>   
> @@ -116,12 +117,13 @@ space_def_dup(const struct space_def *src)
>   			if (src->fields[i].default_value != NULL) {
>   				ret->fields[i].default_value = strs_pos;
>   				strs_pos += strlen(strs_pos) + 1;
> -
> -				struct Expr *e =
> -					src->fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				src->fields[i].default_value_expr;
> +			if (e != NULL) {
>   				char *expr_pos_old = expr_pos;
> -				e = sql_expr_dup(sql_get(), e, 0, &expr_pos);
> +				e = sql_expr_dup(sql_get(), e, 0,
> +						 &expr_pos);

2. Garbage diff. You just wrap &expr_pos on the new line, but it fits in one
line with sql_expr_dup.
> @@ -201,12 +203,13 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
>   				       fields[i].default_value, len);
>   				def->fields[i].default_value[len] = 0;
>   				strs_pos += len + 1;
> -
> -				struct Expr *e =
> -					fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				fields[i].default_value_expr;
> +			if (e != NULL) {
>   				char *expr_pos_old = expr_pos;
> -				e = sql_expr_dup(sql_get(), e, 0, &expr_pos);
> +				e = sql_expr_dup(sql_get(), e, 0,
> +						 &expr_pos);

3. Same as 2. Please, before sending a patch make a self-review. It really
helps, I do it too, even for review letters, that I am sending now. During
self-review you can catch and remove such things with no waiting until I
found it myself.
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 166bb71..38aeac6 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1681,3 +1679,48 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
>   
>   	return space->def->fields[fieldno].default_value_expr;
>   }
> +
> +Table *
> +sql_ephemeral_table_new(Parse *parser)
> +{
> +	sqlite3 *db = parser->db;
> +	struct space_def *def = NULL;
> +	Table *table = sqlite3DbMallocZero(db, sizeof(Table));
> +	if (table != NULL) {
> +		def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
> +				    &space_opts_default, NULL, 0);
> +	}
> +	if (def == NULL) {
> +		sqlite3DbFree(db, table);
> +		parser->rc = SQLITE_NOMEM_BKPT;
> +		parser->nErr++;
> +		return NULL;
> +	}

4. Table can be NULL, and def not NULL - it leads to crash. And why you do not
call sqlite3OomFault(db); here?
> diff --git a/src/box/sql.h b/src/box/sql.h
> index db92d80..9fb3ad1 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -65,6 +65,7 @@ sql_get();
>   struct Expr;
>   struct Parse;
>   struct Select;
> +struct Table;
>   
>   /**
>    * Perform parsing of provided expression. This is done by
> @@ -143,6 +144,28 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer);
>   void
>   sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
>   
> +/**
> + * Create and initialize a new ephemeric SQL Table object.

5. As I said in the previous reviews, 'ephemeric' word does not mean
'эфемерный'. Use ephemeral.

> + * @param pParse SQL Parser object.
> + * @param zName Table to create name.

6. I do not see zName parameter here.

> + * @retval NULL on memory allocation error, Parser state changed.
> + * @retval not NULL on success.
> + */
> +struct Table *
> +sql_ephemeral_table_new(struct Parse *parser);
> +
> +/**
> + * Rebuild struct def in Table with memory allocated on a single
> + * malloc. Fields and strings are expected to be allocated with
> + * sqlite3DbMalloc.
> + * @param db The database connection.
> + * @param pTable The Table with fragmented def to rebuild.
> + * @retval 1 on memory allocation error

7. Please, return -1 on errors.
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 24f0965..bedf602 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -144,6 +144,9 @@ sqlite3AlterRenameTable(Parse * pParse,	/* Parser context. */
>   void
>   sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
>   {
> +	/* This function is not implemented yet #3075. */
> +	assert(false);

8. We have 'unreachable()' macro for this.