Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v3 2/4] sql: Remove zName and nColumn from SQL.
Date: Thu, 26 Apr 2018 14:47:21 +0300	[thread overview]
Message-ID: <d47e168e-23e1-04f9-084b-c9933b1ff02c@tarantool.org> (raw)
In-Reply-To: <ea3a7c37e59c25102fe93deab36aa7817b710245.1524675029.git.kshcherbatov@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.

  parent reply	other threads:[~2018-04-26 11:47 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-25 16:52 [tarantool-patches] [PATCH v3 0/4] sql: Removed Column fields to server with region allocations Kirill Shcherbatov
2018-04-25 16:52 ` [tarantool-patches] [PATCH v3 1/4] sql: Fix code style in sqlite3Pragma Kirill Shcherbatov
2018-04-26 11:47   ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-25 16:52 ` [tarantool-patches] [PATCH v3 2/4] sql: Remove zName and nColumn from SQL Kirill Shcherbatov
2018-04-25 17:10   ` [tarantool-patches] " Kirill Shcherbatov
2018-04-26 12:12     ` Vladislav Shpilevoy
2018-04-26 11:47   ` Vladislav Shpilevoy [this message]
2018-04-25 16:52 ` [tarantool-patches] [PATCH v3 3/4] sql: Removed type " Kirill Shcherbatov
2018-04-25 16:52 ` [tarantool-patches] [PATCH v3 4/4] sql: Region-based allocations Kirill Shcherbatov
2018-04-26 11:47   ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-26 11:47 ` [tarantool-patches] Re: [PATCH v3 0/4] sql: Removed Column fields to server with region allocations Vladislav Shpilevoy
2018-04-28 18:26 ` [tarantool-patches] [PATCH v4 0/7] sql: refactor SQL Parser structures Kirill Shcherbatov
2018-04-28 18:26   ` [tarantool-patches] [PATCH v4 1/7] sql: fix code style in sqlite3Pragma Kirill Shcherbatov
2018-05-03 10:10     ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-28 18:26   ` [tarantool-patches] [PATCH v4 2/7] sql: remove zName and nColumn from SQL Kirill Shcherbatov
2018-05-03 10:10     ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-28 18:26   ` [tarantool-patches] [PATCH v4 3/7] sql: start using type from space_def Kirill Shcherbatov
2018-04-28 18:26   ` [tarantool-patches] [PATCH v4 4/7] sql: start using collations and is_nullable " Kirill Shcherbatov
2018-05-03 10:21     ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-28 18:26   ` [tarantool-patches] [PATCH v4 5/7] sql: move names to server Kirill Shcherbatov
2018-05-03 11:08     ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-28 18:26   ` [tarantool-patches] [PATCH v4 6/7] sql: start using is_view field from space_def Kirill Shcherbatov
2018-05-03 11:16     ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-28 18:26   ` [tarantool-patches] [PATCH v4 7/7] sql: space_def* instead of Table* in Expr Kirill Shcherbatov
2018-05-03 11:32     ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-03 10:10   ` [tarantool-patches] Re: [PATCH v4 0/7] sql: refactor SQL Parser structures Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d47e168e-23e1-04f9-084b-c9933b1ff02c@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=kshcherbatov@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v3 2/4] sql: Remove zName and nColumn from SQL.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox