From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id D3D802A84D for ; Mon, 16 Apr 2018 15:23:24 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id M3Fl28QmyzuL for ; Mon, 16 Apr 2018 15:23:24 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8DB152A500 for ; Mon, 16 Apr 2018 15:23:24 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure. References: <60fd0daec8e35563602212ca8a3307ab49b14d15.1523896524.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <71db35b8-be0a-34be-07ab-09d63c4c39cd@tarantool.org> Date: Mon, 16 Apr 2018 22:23:20 +0300 MIME-Version: 1.0 In-Reply-To: <60fd0daec8e35563602212ca8a3307ab49b14d15.1523896524.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Kirill Shcherbatov 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); >