[tarantool-patches] Re: [PATCH v3 2/4] sql: Remove zName and nColumn from SQL.
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Apr 26 14:47:21 MSK 2018
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.
More information about the Tarantool-patches
mailing list