[tarantool-patches] Re: [PATCH v5 3/3] sql: space_def* instead of Table* in Expr

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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.