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 44B0D2532C for ; Fri, 11 May 2018 16:59:54 -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 NtDsF83UFgwZ for ; Fri, 11 May 2018 16:59:54 -0400 (EDT) Received: from smtp17.mail.ru (smtp17.mail.ru [94.100.176.154]) (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 BD59C2531C for ; Fri, 11 May 2018 16:59:53 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 3/3] sql: space_def* instead of Table* in Expr References: From: Vladislav Shpilevoy Message-ID: <3be5bcd1-a445-d39c-8da9-a50799bf7693@tarantool.org> Date: Fri, 11 May 2018 23:59:48 +0300 MIME-Version: 1.0 In-Reply-To: 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! See 12 comments below. On 11/05/2018 11:49, Kirill Shcherbatov wrote: > This patch allows to remove Checks from SQL to > server as sqlite3ResolveSelfReference requires > Expr structure pointer. > > Part of #3272. > --- > src/box/field_def.c | 1 + > src/box/field_def.h | 14 ++++++ > src/box/sql.c | 10 ++--- > src/box/sql/build.c | 48 +++++++++++--------- > src/box/sql/delete.c | 4 +- > src/box/sql/expr.c | 115 +++++++++++++++++++++++++++--------------------- > src/box/sql/fkey.c | 13 +++--- > src/box/sql/insert.c | 39 +++++++++------- > src/box/sql/pragma.c | 8 ++-- > src/box/sql/resolve.c | 10 ++--- > src/box/sql/select.c | 26 ++++++----- > src/box/sql/sqliteInt.h | 41 +++++------------ > src/box/sql/update.c | 35 +++++++-------- > src/box/sql/vdbeaux.c | 28 +++--------- > src/box/sql/where.c | 13 +++--- > src/box/sql/wherecode.c | 19 +++++--- > src/box/sql/whereexpr.c | 2 +- > 17 files changed, 219 insertions(+), 207 deletions(-) > > diff --git a/src/box/field_def.h b/src/box/field_def.h > index a42beab..b210756 100644 > --- a/src/box/field_def.h > +++ b/src/box/field_def.h > @@ -70,6 +70,15 @@ enum on_conflict_action { > on_conflict_action_MAX > }; > > +enum affinity_type { > + SQLITE_AFF_UNDEFINED = 0, > + SQLITE_AFF_BLOB = 'A', > + SQLITE_AFF_TEXT = 'B', > + SQLITE_AFF_NUMERIC = 'C', > + SQLITE_AFF_INTEGER = 'D', > + SQLITE_AFF_REAL = 'E', 1. Please, do not use SQLITE prefix in Tarantool. Lets name it with AFFINITY prefix instead of SQLITE_AFF. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index a02fe89..f082d01 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1049,10 +1051,10 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken) > * @retval Pointer to collation. > */ > struct coll * > -sql_column_collation(Table *table, uint32_t column) > +sql_column_collation(struct space_def *def, uint32_t column) 2. Fix the comment as well. 3. Now this function is useless. You always can get the collation from coll_by_id(def->fields[column].coll_id). > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 119940c..8b34c57 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -3175,8 +3180,9 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */ > struct Index *pk = sqlite3PrimaryKeyIndex(tab); > assert(pk); > > + char affinity = tab->def->fields[pk->aiColumn[0]].affinity; > if (pk->nColumn == 1 > - && tab->aCol[pk->aiColumn[0]].affinity == 'D' > + && affinity == 'D' 4. Lets do not inline affinity values: use AFFINITY_INTEGER. > @@ -3519,45 +3525,48 @@ sqlite3ExprCodeLoadIndexColumn(Parse * pParse, /* The parsing context */ > sqlite3ExprCodeCopy(pParse, pIdx->aColExpr->a[iIdxCol].pExpr, > regOut); > } else { > - sqlite3ExprCodeGetColumnOfTable(pParse->pVdbe, pIdx->pTable, > + sqlite3ExprCodeGetColumnOfTable(pParse->pVdbe, pIdx->pTable->def, > iTabCur, iTabCol, regOut); > } > } > > -/* > +/** > * Generate code to extract the value of the iCol-th column of a table. > + * @param v The VDBE under construction. 5. To many white spaces. > + * @param space_def Space definition. > + * @param iTabCur The PK cursor. > + * @param iCol Index of the column to extract. > + * @param regOut Extract the value into this register. > */ > void > -sqlite3ExprCodeGetColumnOfTable(Vdbe * v, /* The VDBE under construction */ > - Table * pTab, /* The table containing the value */ > - int iTabCur, /* The PK cursor */ > - int iCol, /* Index of the column to extract */ > - int regOut /* Extract the value into this register */ > - ) > +sqlite3ExprCodeGetColumnOfTable(Vdbe * v, struct space_def *space_def, 6. Do not put white space after '*' in a declaration. Here and in other places. > + int iTabCur, int iCol, int regOut) > { > sqlite3VdbeAddOp3(v, OP_Column, iTabCur, iCol, regOut); > if (iCol >= 0) { > - sqlite3ColumnDefault(v, pTab, iCol, regOut); > + sqlite3ColumnDefault(v, space_def, iCol, regOut); > } > } > > -/* > +/** > * Generate code that will extract the iColumn-th column from > * table pTab and store the column value in a register. > * > - * An effort is made to store the column value in register iReg. This > - * is not garanteeed for GetColumn() - the result can be stored in > - * any register. But the result is guaranteed to land in register iReg > - * for GetColumnToReg(). > + * An effort is made to store the column value in register iReg. > + * This is not garanteeed for GetColumn() - the result can be > + * stored in any register. But the result is guaranteed to land > + * in register iReg for GetColumnToReg(). > + * @param pParse Parsing and code generating context. > + * @param space_def Space definition. > + * @param iColumn Index of the table column. > + * @param iTable The cursor pointing to the table. > + * @param iReg Store results here. > + * @param p5 P5 value for OP_Column + FLAGS. > + * @return iReg value. > */ > int > -sqlite3ExprCodeGetColumn(Parse * pParse, /* Parsing and code generating context */ > - Table * pTab, /* Description of the table we are reading from */ > - int iColumn, /* Index of the table column */ > - int iTable, /* The cursor pointing to the table */ > - int iReg, /* Store results here */ > - u8 p5 /* P5 value for OP_Column + FLAGS */ > - ) > +sqlite3ExprCodeGetColumn(Parse * pParse, struct space_def * space_def, > + int iColumn, int iTable, int iReg, u8 p5) 7. Same. > diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c > index 250c402..0207ee8 100644 > --- a/src/box/sql/pragma.c > +++ b/src/box/sql/pragma.c > @@ -373,7 +373,9 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ > i; k++) { > } > } > - bool nullable = table_column_is_nullable(pTab, i); > + bool nullable = > + space_def_column_is_nullable( > + pTab->def, i); 8. For flags use 'is_' prefix, please. I know, it is not your code, but if you modify it, then lets do it correctly. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 32a8e08..5bd958a 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 4fba008..ad7cc54 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -2226,7 +2205,8 @@ struct AggInfo { > int mnReg, mxReg; /* Range of registers allocated for aCol and aFunc */ > ExprList *pGroupBy; /* The group by clause */ > struct AggInfo_col { /* For each column used in source tables */ > - Table *pTab; /* Source table */ > + /* Pointer to space definition. */ > + struct space_def *space_def; 9. Please, use /** prefix for comments out of function body. > @@ -2361,7 +2341,8 @@ struct Expr { > * TK_AGG_FUNCTION: nesting depth > */ > AggInfo *pAggInfo; /* Used by TK_AGG_COLUMN and TK_AGG_FUNCTION */ > - Table *pTab; /* Table for TK_COLUMN expressions. */ > + /* Pointer for table relative definition. */ 10. Same. > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 5f5807c..9ae77e0 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -69,30 +69,27 @@ > * space. > */ > void > -sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, int iReg) > +sqlite3ColumnDefault(Vdbe * v, struct space_def * def, int i, int iReg) 11. Same as 5. > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index 0ccca77..3054fc3 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -4724,28 +4724,10 @@ table_column_nullable_action(struct Table *tab, uint32_t column) > * @return return nullability flag value > */ > bool > -table_column_is_nullable(struct Table *tab, uint32_t column) > +space_def_column_is_nullable(struct space_def *def, uint32_t column) > { > - /* Temporary hack: until Tarantoool's ephemeral spaces are on-boarded, > - * views are not handled properly in Tarantool as well. */ > - if (!(tab->tabFlags | TF_Ephemeral || space_is_view(tab))) { > - uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(tab->tnum); > - struct space *space = space_cache_find(space_id); > - > - assert(space); > - > - struct tuple_format *format = space->format; > - > - assert(format); > - assert(format->field_count > column); > - > - return nullable_action_to_is_nullable( > - format->fields[column].nullable_action); > - } else { > - /* tab is ephemeral (in SQLite sense). */ > - assert(tab->def->fields[column].is_nullable == > - nullable_action_to_is_nullable( > - tab->def->fields[column].nullable_action)); > - return tab->def->fields[column].is_nullable; > - } > + assert(def->fields[column].is_nullable == > + nullable_action_to_is_nullable( > + def->fields[column].nullable_action)); > + return def->fields[column].is_nullable; 12. Perfect. Now this function is useless. It can be inlined and removed.