[tarantool-patches] Re: [PATCH v4 7/7] sql: space_def* instead of Table* in Expr
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu May 3 14:32:34 MSK 2018
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.
More information about the Tarantool-patches
mailing list