[tarantool-patches] Re: [PATCH v5 3/3] sql: space_def* instead of Table* in Expr
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri May 11 23:59:48 MSK 2018
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.
More information about the Tarantool-patches
mailing list