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

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

Thanks for the fixes! See 5 comments below.

> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 5676d0720..db8577d1c 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -252,74 +244,57 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id)
> -/*
> - * 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 field_type is the type of an indexed column.
> + * @retval Return true if the index with @field_type may be used to
> + * implement the comparison in expr.
>   */
> -int
> -sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity)
> +bool
> +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)
>  {
> -	char aff = comparisonAffinity(pExpr);
> -	switch (aff) {
> -	case AFFINITY_BLOB:
> -		return 1;
> -	case AFFINITY_TEXT:
> -		return idx_affinity == AFFINITY_TEXT;
> +	enum field_type type = expr_cmp_mutual_type(expr);
> +	switch (type) {
> +	case FIELD_TYPE_SCALAR:
> +		return true;
> +	case FIELD_TYPE_STRING:
> +		return field_type == FIELD_TYPE_STRING;

1. But type is never == FIELD_TYPE_STRING here. Because
expr_cmp_mutual_type() returns either SCALAR or NUMBER.
I added an assertion type != FIELD_TYPE_STRING and it did
not fail in tests.

>  	default:
> -		return sqlite3IsNumericAffinity(idx_affinity);
> +		return sql_type_is_numeric(field_type);

2. Also, I do not understand, why not to just use
field_type1_contains_type2(). sql_expr_cmp_type_is_compatible()
says, that it returns true, if a column of type field_type can
be used to "implement comparison" in an expression. So
this is the same as

     field_type1_contains_type2(field_type, expr_cmp_mutual_type(expr));

If I correctly understood what does it mean - "implement comparison".
Then I tried to replace sql_expr_cmp_type_is_compatible() with the
call above and the tests failed. Then I swapped the arguments:

     field_type1_contains_type2(expr_cmp_mutual_type(expr), field_type);

And the tests passed. Why?

>  	}
>  }
>  
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index ef4514374..9843eb4a5 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1728,38 +1727,31 @@ generateColumnNames(Parse * pParse,	/* Parser context */
>  		p = pEList->a[i].pExpr;
>  		if (NEVER(p == 0))
>  			continue;
> -		switch (p->affinity) {
> -		case AFFINITY_INTEGER:
> +		if (p->op == TK_VARIABLE)
> +			var_pos[var_count++] = i;

3. Why did you move these two lines out of 'switch:' clause? As I
understand, for field types INTEGER, NUMBER, STRING, SCALAR op
never == TK_VARIABLE. So this check now executes mostly without
necessity, and should be done only for FIELD_TYPE_BOOLEAN. I
did this and the tests passed:

	diff --git a/src/box/sql/select.c b/src/box/sql/select.c
	index 9843eb4a5..d8ab9a40d 100644
	--- a/src/box/sql/select.c
	+++ b/src/box/sql/select.c
	@@ -1727,8 +1727,6 @@ generateColumnNames(Parse * pParse,	/* Parser context */
	 		p = pEList->a[i].pExpr;
	 		if (NEVER(p == 0))
	 			continue;
	-		if (p->op == TK_VARIABLE)
	-			var_pos[var_count++] = i;
	 		switch (p->type) {
	 		case FIELD_TYPE_INTEGER:
	 			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER",
	@@ -1747,6 +1745,8 @@ generateColumnNames(Parse * pParse,	/* Parser context */
	 					      SQLITE_TRANSIENT);
	 			break;
	 		case FIELD_TYPE_BOOLEAN:
	+			if (p->op == TK_VARIABLE)
	+				var_pos[var_count++] = i;
	 			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BOOLEAN",
	 					      SQLITE_TRANSIENT);
	 			break;

> +		switch (p->type) {
> +		case FIELD_TYPE_INTEGER:
>  			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "INTEGER",
>  					      SQLITE_TRANSIENT);
>  			break;
> -		case AFFINITY_REAL:
> +		case FIELD_TYPE_NUMBER:
>  			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "NUMERIC",
>  					      SQLITE_TRANSIENT);
>  			break;
> -		case AFFINITY_TEXT:
> +		case FIELD_TYPE_STRING:
>  			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "TEXT",
>  					      SQLITE_TRANSIENT);
>  			break;
> -		case AFFINITY_BLOB:
> +		case FIELD_TYPE_SCALAR:
>  			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BLOB",
>  					      SQLITE_TRANSIENT);
>  			break;
> -		default: ;
> -			char *type;
> -			/*
> -			 * For variables we set BOOLEAN type since
> -			 * unassigned bindings will be replaced
> -			 * with NULL automatically, i.e. without
> -			 * explicit call of sql_bind_value().
> -			 */
> -			if (p->op == TK_VARIABLE) {
> -				var_pos[var_count++] = i;
> -				type = "BOOLEAN";
> -			} else {
> -				type = "UNKNOWN";
> -			}
> -			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, type,
> +		case FIELD_TYPE_BOOLEAN:
> +			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "BOOLEAN",
> +					      SQLITE_TRANSIENT);
> +			break;
> +		default:
> +			sqlite3VdbeSetColName(v, i, COLNAME_DECLTYPE, "UNKNOWN",
>  					      SQLITE_TRANSIENT);
>  		}
>  		if (pEList->a[i].zName) {
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 79999509c..047a77b68 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2091,8 +2093,8 @@ typedef int ynVar;
>  struct Expr {
>  	u8 op;			/* Operation performed by this node */
>  	union {
> -		/** The affinity of the column. */
> -		enum affinity_type affinity;
> +		/** The Type of the column. */

4. 'Type' -> 'type'.

> +		enum field_type type;
>  		/** Conflict action for RAISE() function. */
>  		enum on_conflict_action on_conflict_action;
>  	};
> @@ -4267,26 +4281,30 @@ sql_index_type_str(struct sqlite3 *db, const struct index_def *idx_def);
>  /**
> - * 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.
> + *
> + * 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);

5. Expr -> struct Expr.