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

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

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.