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 A65A920EC3 for ; Thu, 3 May 2018 07:38:37 -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 eBnJVKo-Olbe for ; Thu, 3 May 2018 07:38:37 -0400 (EDT) Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 4A0D920E8C for ; Thu, 3 May 2018 07:38:36 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 7/7] sql: space_def* instead of Table* in Expr References: <5ec6ab0d77b24efbed08699c96afd04bd0cf0739.1524939875.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 3 May 2018 14:32:34 +0300 MIME-Version: 1.0 In-Reply-To: <5ec6ab0d77b24efbed08699c96afd04bd0cf0739.1524939875.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. Write here why it is needed, and what this patch allows to do in the next step. See 10 comments below. On 28/04/2018 21:26, Kirill Shcherbatov wrote: > Part of #3272. > --- > src/box/field_def.c | 1 + > src/box/field_def.h | 5 ++++ > src/box/sql.c | 10 ++++---- > src/box/sql/build.c | 48 ++++++++++++++++++++------------------ > src/box/sql/delete.c | 4 ++-- > src/box/sql/expr.c | 61 ++++++++++++++++++++++++++----------------------- > src/box/sql/fkey.c | 13 +++++------ > src/box/sql/insert.c | 24 ++++++++++--------- > src/box/sql/pragma.c | 8 ++++--- > src/box/sql/resolve.c | 10 ++++---- > src/box/sql/select.c | 26 +++++++++++++-------- > src/box/sql/sqliteInt.h | 21 +++++++++-------- > src/box/sql/update.c | 29 +++++++++++------------ > src/box/sql/vdbeaux.c | 20 ++++++---------- > src/box/sql/where.c | 13 +++++++---- > src/box/sql/wherecode.c | 18 +++++++++------ > src/box/sql/whereexpr.c | 2 +- > 17 files changed, 167 insertions(+), 146 deletions(-) > > diff --git a/src/box/field_def.c b/src/box/field_def.c > index 010b3b7..63aab46 100644 > --- a/src/box/field_def.c > +++ b/src/box/field_def.c > @@ -100,6 +100,7 @@ const struct opt_def field_def_reg[] = { > > const struct field_def field_def_default = { > .type = FIELD_TYPE_ANY, > + .affinity = 0, 1. Use enum for affinities. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 119940c..4e20098 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -179,13 +181,13 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found) > } > if ((op == TK_AGG_COLUMN || op == TK_COLUMN || > op == TK_REGISTER || op == TK_TRIGGER) && > - p->pTab != 0) { > + p->space_def != 0) { > /* op==TK_REGISTER && p->pTab!=0 happens when pExpr was originally > * a TK_COLUMN but was previously evaluated and cached in a register > */ > int j = p->iColumn; > if (j >= 0) { > - coll = sql_column_collation(p->pTab, j); > + coll = sql_column_collation(p->space_def, j); > *is_found = true; > } > break; > @@ -2132,10 +2134,10 @@ sqlite3ExprCanBeNull(const Expr * p) > case TK_BLOB: > return 0; > case TK_COLUMN: > - assert(p->pTab != 0); > + assert(p->space_def!= 0); 2. Use explicit != NULL, and put white space around binary operators. > @@ -3529,7 +3534,7 @@ sqlite3ExprCodeLoadIndexColumn(Parse * pParse, /* The parsing context */ > */ > void > sqlite3ExprCodeGetColumnOfTable(Vdbe * v, /* The VDBE under construction */ > - Table * pTab, /* The table containing the value */ > + struct space_def *space_def, 3. Please, convert the function into Tarantool code style, and write a comment. Same about all functions, whose signature is changed. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index bde0cc1..7c21359 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -1805,17 +1807,17 @@ xferOptimization(Parse * pParse, /* Parser context */ > return 0; /* Both tables must have the same INTEGER PRIMARY KEY */ > } > for (i = 0; i < (int)pDest->def->field_count; i++) { > - Column *pDestCol = &pDest->aCol[i]; > - Column *pSrcCol = &pSrc->aCol[i]; > - if (pDestCol->affinity != pSrcCol->affinity) { > + char pdest_affinity = pDest->def->fields[i].affinity; > + char psrc_affinity = pSrc->def->fields[i].affinity; > + if (pdest_affinity != psrc_affinity) { > return 0; /* Affinity must be the same on all columns */ > } 4. Unnecessary {}. > - if (sql_column_collation(pDest, i) != > - sql_column_collation(pSrc, i)) { > + if (sql_column_collation(pDest->def, i) != > + sql_column_collation(pSrc->def, i)) { > return 0; /* Collating sequence must be the same on all columns */ > } 5. Same. > - if (!table_column_is_nullable(pDest, i) > - && table_column_is_nullable(pSrc, i)) { > + if (!table_column_is_nullable(pDest->def, i) > + && table_column_is_nullable(pSrc->def, i)) { > return 0; /* tab2 must be NOT NULL if tab1 is */ > } 6. Same. > diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c > index 463bb7e..c49817f 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 = > + table_column_is_nullable(pTab->def, > + i); 7. Now this function can not be names table_column.... It does not take table as an argument. Please, rename it to space_def. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 34d296d..ff9f18b 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1636,7 +1636,7 @@ columnTypeImpl(NameContext * pNC, Expr * pExpr, > break; > } > > - assert(pTab && pExpr->pTab == pTab); > + assert(pTab && pExpr->space_def == pTab->def); 8. Please, use explicit != NULL. > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index 2f36423..38f824f 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) > { > - assert(pTab != 0); > - assert(pTab->def->opts.is_view == (pTab->pSelect != NULL)); > - if (!pTab->def->opts.is_view) { > + assert(def != NULL); > + if (!def->opts.is_view) { > sqlite3_value *pValue = 0; > - Column *pCol = &pTab->aCol[i]; > - VdbeComment((v, "%s.%s", pTab->def->name, > - pTab->def->fields[i].name)); > - assert(i < (int)pTab->def->field_count); > + char affinity = def->fields[i].affinity; > + VdbeComment((v, "%s.%s", def->name, def->fields[i].name)); > + assert(i < (int)def->field_count); > > Expr *expr = NULL; > struct space *space = > - space_cache_find(SQLITE_PAGENO_TO_SPACEID(pTab->tnum)); > + space_cache_find(def->id); 9. Why do you still need space_cache_find, if you already have space_def and default_value_expr in it? > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index f76c689..f9927ed 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -4724,28 +4724,22 @@ 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) > +table_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 space *space = space_cache_find(def->id); 10. You do not need space_cache_find. Space_def is enough here, it has nullable action too.