[tarantool-patches] Re: [PATCH 6/8] sql: replace affinity with field type in struct Expr

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

Thanks for the patch! See 10 comments below.

On 28/12/2018 12:34, Nikita Pettik wrote:
> Also this patch resolves issue connected with wrong query plans during
> select on spaces created from Lua: instead of index search in most cases
> table scan was used. It appeared due to the fact that index was checked
> on affinity compatibility with space format. So, if space is created
> without affinity in format, indexes won't be used.
> However, now all checks are related to field types, and as a result
> query optimizer is able to choose correct index.
> 
> Closes #3886
> Part of #3698
> ---
>   src/box/sql/build.c              |   6 +-
>   src/box/sql/expr.c               | 270 +++++++++++++++++----------------------
>   src/box/sql/fkey.c               |   5 +-
>   src/box/sql/insert.c             |   2 +-
>   src/box/sql/parse.y              |  12 +-
>   src/box/sql/resolve.c            |   4 +-
>   src/box/sql/select.c             |  23 ++--
>   src/box/sql/sqliteInt.h          |  46 ++++---
>   src/box/sql/vdbemem.c            |  18 +--
>   src/box/sql/where.c              |  25 ++--
>   src/box/sql/whereInt.h           |   3 +-
>   src/box/sql/wherecode.c          |  38 +++---
>   src/box/sql/whereexpr.c          |  31 ++---
>   test/sql-tap/lua-tables.test.lua |  38 +++++-
>   test/sql/iproto.result           |   2 +-
>   15 files changed, 257 insertions(+), 266 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index c823c5a06..32606dac3 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -256,28 +231,29 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
>   	return coll;
>   }
>   
> -enum affinity_type
> -sql_affinity_result(enum affinity_type aff1, enum affinity_type aff2)
> +enum field_type
> +sql_type_result(enum field_type lhs, enum field_type rhs)
>   {
> -	if (aff1 && aff2) {
> -		/* Both sides of the comparison are columns. If one has numeric
> -		 * affinity, use that. Otherwise use no affinity.
> +	if (lhs != FIELD_TYPE_ANY && rhs != FIELD_TYPE_ANY) {
> +		/*
> +		 * Both sides of the comparison are columns.
> +		 * If one has numeric type, use that.
>   		 */

1. The comment is useless IMO. It just repeats exactly the
same obvious thing what is written in the next line.

Writing comments, keep in mind our meme:

https://github.com/Gerold103/tarantool-memes/blob/master/CAPTAIN.png

Also, I've just understood that we can post memes as a links to
my meme repository! We are saved!

> -		if (sqlite3IsNumericAffinity(aff1)
> -		    || sqlite3IsNumericAffinity(aff2)) {
> -			return AFFINITY_REAL;
> -		} else {
> -			return AFFINITY_BLOB;
> -		}
> -	} else if (!aff1 && !aff2) {
> -		/* Neither side of the comparison is a column.  Compare the
> -		 * results directly.
> +		if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
> +			return FIELD_TYPE_NUMBER;
> +		else
> +			return FIELD_TYPE_SCALAR;
> +

2. Unnecessary empty line.

> +	} else if (lhs == FIELD_TYPE_ANY && rhs == FIELD_TYPE_ANY) {
> +		/*
> +		 * Neither side of the comparison is a column.
> +		 * Compare the results directly.
>   		 */
> -		return AFFINITY_BLOB;
> +		return FIELD_TYPE_SCALAR;
>   	} else {
> -		/* One side is a column, the other is not. Use the columns affinity. */
> -		assert(aff1 == 0 || aff2 == 0);
> -		return (aff1 + aff2);
> +		if (lhs == FIELD_TYPE_ANY)
> +			return rhs;
> +		return lhs;
>   	}
>   }
>   
> @@ -285,45 +261,44 @@ sql_affinity_result(enum affinity_type aff1, enum affinity_type aff2)
>    * pExpr is a comparison operator.  Return the type affinity that should
>    * be applied to both operands prior to doing the comparison.
>    */
> -static char
> +static enum field_type
>   comparisonAffinity(Expr * pExpr)

3. But it is not affinity anymore.

>   {
> -	char aff;
>   	assert(pExpr->op == TK_EQ || pExpr->op == TK_IN || pExpr->op == TK_LT ||
>   	       pExpr->op == TK_GT || pExpr->op == TK_GE || pExpr->op == TK_LE ||
>   	       pExpr->op == TK_NE);
>   	assert(pExpr->pLeft);
> -	aff = sqlite3ExprAffinity(pExpr->pLeft);
> +	enum field_type type = sql_expr_type(pExpr->pLeft);
>   	if (pExpr->pRight) {
> -		enum affinity_type rhs_aff = sqlite3ExprAffinity(pExpr->pRight);
> -		aff = sql_affinity_result(rhs_aff, aff);
> +		enum field_type rhs_type = sql_expr_type(pExpr->pRight);
> +		type = sql_type_result(rhs_type, type);
>   	} else if (ExprHasProperty(pExpr, EP_xIsSelect)) {
> -		enum affinity_type rhs_aff =
> -			sqlite3ExprAffinity(pExpr->x.pSelect->pEList->a[0].pExpr);
> -		aff = sql_affinity_result(rhs_aff, aff);
> +		enum field_type rhs_type =
> +			sql_expr_type(pExpr->x.pSelect->pEList->a[0].pExpr);
> +		type = sql_type_result(rhs_type, type);
>   	} else {
> -		aff = AFFINITY_BLOB;
> +		type = FIELD_TYPE_SCALAR;
>   	}
> -	return aff;
> +	return type;
>   }
>   
> -/*
> - * pExpr is a comparison expression, eg. '=', '<', IN(...) etc.
> - * idx_affinity is the affinity of an indexed column. Return true
> - * if the index with affinity idx_affinity may be used to implement
> - * the comparison in pExpr.
> +/**
> + * @param expr is a comparison expression, eg. '=', '<', IN(...) etc.
> + * @param idx_affinity is the affinity of an indexed column.
> + * @retval Return true if the index with @idx_type may be used to
> + * implement the comparison in expr.
>    */
> -int
> -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity)
> +enum field_type
> +sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type)

4. Strictly speaking, it is not idx_type. It is a type of one
column. I think, it should be renamed to just type, or field_type,
as well as in struct WhereScan.

Also, sorry, but I hate this suffix '_ok' or 'Ok'. It just says
nothing about what this function does. As well as that it has
nothing to do with an index, but has 'index' in the name.

Maybe rename to something like 'sql_expr_cmp_type_is_possible' ?

>   {
> -	char aff = comparisonAffinity(pExpr);
> -	switch (aff) {
> -	case AFFINITY_BLOB:
> +	enum field_type type = comparisonAffinity(expr);
> +	switch (type) {
> +	case FIELD_TYPE_SCALAR:
>   		return 1;
> -	case AFFINITY_TEXT:
> -		return idx_affinity == AFFINITY_TEXT;
> +	case FIELD_TYPE_STRING:
> +		return idx_type == FIELD_TYPE_STRING;
>   	default:
> -		return sqlite3IsNumericAffinity(idx_affinity);
> +		return sql_type_is_numeric(idx_type);
>   	}
>   }
>   
> @@ -2155,10 +2130,10 @@ sqlite3ExprCanBeNull(const Expr * p)
>    * answer.
>    */
>   int
> -sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
> +sql_expr_needs_type_change(const Expr *p, enum field_type type)

5. Name is changed from negative to positive, but the logic is
the same, why? Original was 'NoChange', now it is 'needs_change'.

Also, please, move the function comment to the declaration in sqliteInt.h.

>   {
>   	u8 op;
> -	if (aff == AFFINITY_BLOB)
> +	if (type == FIELD_TYPE_SCALAR)
>   		return 1;
>   	while (p->op == TK_UPLUS || p->op == TK_UMINUS) {
>   		p = p->pLeft;
> @@ -2167,27 +2142,21 @@ sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
>   	if (op == TK_REGISTER)
>   		op = p->op2;
>   	switch (op) {
> -	case TK_INTEGER:{
> -			return aff == AFFINITY_INTEGER;
> -		}
> -	case TK_FLOAT:{
> -			return aff == AFFINITY_REAL;
> -		}
> -	case TK_STRING:{
> -			return aff == AFFINITY_TEXT;
> -		}
> -	case TK_BLOB:{
> -			return 1;
> -		}
> +	case TK_INTEGER:
> +		return type == FIELD_TYPE_INTEGER;
> +	case TK_FLOAT:
> +		return type == FIELD_TYPE_NUMBER;
> +	case TK_STRING:
> +		return type == FIELD_TYPE_STRING;
> +	case TK_BLOB:
> +		return 1;
>   	case TK_COLUMN:{
> -			assert(p->iTable >= 0);	/* p cannot be part of a CHECK constraint */
> -			return p->iColumn < 0
> -			    && (aff == AFFINITY_INTEGER
> -				|| aff == AFFINITY_REAL);
> -		}
> -	default:{
> -			return 0;
> +			/* p cannot be part of a CHECK constraint. */
> +			assert(p->iTable >= 0);
> +			return p->iColumn < 0 && sql_type_is_numeric(type);
>   		}

6. Please, remove these braces too, if you decided to refactor
sqlite3ExprNeedsNoAffinityChange.

> +	default:
> +		return 0;
>   	}
>   }
>   
> @@ -2826,16 +2795,15 @@ sqlite3CodeSubselect(Parse * pParse,	/* Parsing context */
>   				 * that columns affinity when building index keys. If <expr> is not
>   				 * a column, use numeric affinity.
>   				 */
> -				char affinity;	/* Affinity of the LHS of the IN */
>   				int i;
>   				ExprList *pList = pExpr->x.pList;
>   				struct ExprList_item *pItem;
>   				int r1, r2, r3;
>   
> -				affinity = sqlite3ExprAffinity(pLeft);
> -				if (!affinity) {
> -					affinity = AFFINITY_BLOB;
> -				}
> +				enum field_type lhs_type =
> +					sql_expr_type(pLeft);
> +				if (lhs_type == FIELD_TYPE_ANY)
> +					lhs_type = FIELD_TYPE_SCALAR;

7. Why did not you move this conversion to sql_expr_type? I mean
ANY -> SCALAR. ANY differs from SCALAR in only one thing - it is
able to store MP_MAP and MP_ARRAY. So I am slightly bent upon
frequency of ANY usage in SQL, wherein MAP/ARRAY does not exist.

>   				bool unused;
>   				sql_expr_coll(pParse, pExpr->pLeft,
>   					      &unused, &key_info->parts[0].coll_id);
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 690fa6431..7d1159345 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2092,7 +2094,7 @@ struct Expr {
>   	u8 op;			/* Operation performed by this node */
>   	union {
>   		/** The affinity of the column. */

8. Not affinity anymore.

> -		enum affinity_type affinity;
> +		enum field_type type;
>   		/** Conflict action for RAISE() function. */
>   		enum on_conflict_action on_conflict_action;
>   	};
> @@ -4253,26 +4257,30 @@ sql_index_type_str(struct sqlite3 *db, const struct index_def *idx_def);
>   void
>   sql_emit_table_types(struct Vdbe *v, struct space_def *def, int reg);
>   
> -/**
> - * Return superposition of two affinities.
> - * This may be required for determining resulting
> - * affinity of expressions like a + '2'.
> - */
> -enum affinity_type
> -sql_affinity_result(enum affinity_type aff1, enum affinity_type aff2);
> +enum field_type
> +sql_type_result(enum field_type lhs, enum field_type rhs);
>   
> -int sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity);
> +enum field_type
> +sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type);
>   
>   /**
> - * Return the affinity character for a single column of a table.
> - * @param def space definition.
> - * @param idx column index.
> - * @retval AFFINITY
> + * Return the type of the expression pExpr.
> + *
> + * If pExpr is a column, a reference to a column via an 'AS' alias,
> + * or a sub-select with a column as the return value, then the
> + * type of that column is returned. Otherwise, type ANY is returned,
> + * indicating that the expression can feature any type.

9. Returning to point 7 - I do not think it should return ANY. Anyway
you convert it to SCALAR in all places.

> + *
> + * The WHERE clause expressions in the following statements all
> + * have an type:
> + *
> + * CREATE TABLE t1(a);
> + * SELECT * FROM t1 WHERE a;
> + * SELECT a AS b FROM t1 WHERE b;
> + * SELECT * FROM t1 WHERE (select a from t1);
>    */
> -char
> -sqlite3TableColumnAffinity(struct space_def *def, int idx);
> -
> -char sqlite3ExprAffinity(Expr * pExpr);
> +enum field_type
> +sql_expr_type(Expr *pExpr);
>   
>   
>   /**
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index aae5d6617..06335bcf7 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -392,6 +392,7 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff)
>   		base++;
>   		zAff++;
>   	}
> +
>   	while (n > 1 && zAff[n - 1] == AFFINITY_BLOB) {

10. Garbage diff.

>   		n--;
>   	}