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

["n.pettik" <korablev@tarantool.org>]

>>  -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.

Ok, I’d rather agree. I simply copied that comment from
original SQLite function. Now removed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 4816310c3..ba7269d08 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -235,15 +235,10 @@ enum field_type
 sql_type_result(enum field_type lhs, enum field_type rhs)
 {
        if (lhs != FIELD_TYPE_ANY && rhs != FIELD_TYPE_ANY) {
-               /*
-                * Both sides of the comparison are columns.
-                * If one has numeric type, use that.
-                */

> 
>> -		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.

Removed:

                if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
                        return FIELD_TYPE_NUMBER;
                else
                        return FIELD_TYPE_SCALAR;
-
        } else if (lhs == FIELD_TYPE_ANY && rhs == FIELD_TYPE_ANY) {
                /*
                 * Neither side of the comparison is a column.

> 
>> +	} 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.

Renamed:

-/*
- * pExpr is a comparison operator.  Return the type affinity that should
- * be applied to both operands prior to doing the comparison.
+/**
+ * pExpr is a comparison operator. Return the type affinity
+ * that should be applied to both operands prior to doing
+ * the comparison.
  */
 static enum field_type
-comparisonAffinity(Expr * pExpr)
+comparison_mutual_type(struct Expr *pExpr)
 {
        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 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.

Yea, it’s fair. Renamed:

-/*
- * pExpr is a comparison operator.  Return the type affinity that should
- * be applied to both operands prior to doing the comparison.
+/**
+ * pExpr is a comparison operator. Return the type affinity
+ * that should be applied to both operands prior to doing
+ * the comparison.
  */
 static enum field_type
-comparisonAffinity(Expr * pExpr)
+expr_cmp_mutual_type(struct Expr *pExpr)
 {
        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 ||

> 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’ ?

Done:

@@ -289,9 +285,9 @@ comparisonAffinity(Expr * pExpr)
  * implement the comparison in expr.
  */
 enum field_type
-sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type)
+sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type idx_type)
 {
-       enum field_type type = comparisonAffinity(expr);
+       enum field_type type = expr_cmp_mutual_type(expr);
        switch (type) {
        case FIELD_TYPE_SCALAR:
                return 1;
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 216e8d57f..7116f34c5 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4261,7 +4261,7 @@ enum field_type
 sql_type_result(enum field_type lhs, enum field_type rhs);
 
 enum field_type
-sql_index_type_is_ok(struct Expr *expr, enum field_type idx_type);
+sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type idx_type);
 
 /**
  * Return the type of the expression pExpr.
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 54531a8a5..a2e55e8ac 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -298,7 +298,7 @@ whereScanNext(WhereScan * pScan)
                                                /* Verify the affinity and collating sequence match */
                                                if ((pTerm->eOperator & WO_ISNULL) == 0) {
                                                        pX = pTerm->pExpr;
-                                                       if (!sql_index_type_is_ok(pX, pScan->idx_type))
+                                                       if (!sql_expr_cmp_type_is_compatible(pX, pScan->idx_type))
                                                                continue;
                                                        if (pScan->is_column_seen) {
                                                                Parse *pParse =

> 
>>  {
>> -	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’.

My fault, missed ’no’ particle. Fixed:

-/*
- * Return TRUE if the given expression is a constant which would be
- * unchanged by OP_ApplyType with the type given in the second
- * argument.
- *
- * This routine is used to determine if the OP_ApplyType operation
- * can be omitted.  When in doubt return FALSE.  A false negative
- * is harmless.  A false positive, however, can result in the wrong
- * answer.
- */
 int
-sql_expr_needs_type_change(const Expr *p, enum field_type type)
+sql_expr_needs_no_type_change(const Expr *p, enum field_type type)

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 216e8d57f..5541cb6b2 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3710,8 +3710,18 @@ int sqlite3ExprIsTableConstant(Expr *, int);
 int sqlite3ExprIsInteger(Expr *, int *);
 int sqlite3ExprCanBeNull(const Expr *);
 
+/**
+ * Return TRUE if the given expression is a constant which would
+ * be unchanged by OP_ApplyType with the type given in the second
+ * argument.
+ *
+ * This routine is used to determine if the OP_ApplyType operation
+ * can be omitted.  When in doubt return FALSE.  A false negative
+ * is harmless. A false positive, however, can result in the wrong
+ * answer.
+ */
 int
-sql_expr_needs_type_change(const Expr *epr, enum field_type type);
+sql_expr_needs_no_type_change(const Expr *epr, enum field_type type);

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index fe5c56c07..98b55db7a 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -428,7 +428,7 @@ updateRangeAffinityStr(Expr * pRight,       /* RHS of comparison */
                Expr *p = sqlite3VectorFieldSubexpr(pRight, i);
                enum field_type expr_type = sql_expr_type(p);
                if (sql_type_result(expr_type, type) == FIELD_TYPE_SCALAR ||
-                   sql_expr_needs_type_change(p, type)) {
+                       sql_expr_needs_no_type_change(p, type)) {
                        zAff[i] = AFFINITY_BLOB;
                }
        }
@@ -788,7 +788,7 @@ codeAllEqualityTerms(Parse * pParse,        /* Parsing context */
                                    FIELD_TYPE_SCALAR) {
                                        zAff[j] = AFFINITY_BLOB;
                                }
-                               if (sql_expr_needs_type_change(pRight, idx_type))
+                               if (sql_expr_needs_no_type_change(pRight, idx_type))
                                        zAff[j] = AFFINITY_BLOB;
                        }

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

Done, see above.

> 
>>  {
>>  	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.

Ok:

@@ -2150,11 +2136,10 @@ sql_expr_needs_type_change(const Expr *p, enum field_type type)
                return type == FIELD_TYPE_STRING;
        case TK_BLOB:
                return 1;
-       case TK_COLUMN:{
-                       /* p cannot be part of a CHECK constraint. */
-                       assert(p->iTable >= 0);
-                       return p->iColumn < 0 && sql_type_is_numeric(type);
-               }
+       case TK_COLUMN:
+               /* p cannot be part of a CHECK constraint. */
+               assert(p->iTable >= 0);
+               return p->iColumn < 0 && sql_type_is_numeric(type);
        default:

> 
>> +	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.

Unfortunately, right now I can’t replace it with ANY. It is still used
when IN operator comes with one operand (see EP_Generic flag).
I have already sent patch which removes it:
https://github.com/tarantool/tarantool/tree/np/gh-3934-IN-operator-fix

After it hits 2.1, I will replace SCALAR with ANY.

> 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.

But still we can SELECT data from spaces created from Lua.
For instance, _fk_constraint features type ARRAY in its format,
so we can’t ignore this type even now (IMHO).

> 
>>  				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.

Fixed:

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 216e8d57f..209e7bf0f 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2093,7 +2093,7 @@ typedef int ynVar;
 struct Expr {
        u8 op;                  /* Operation performed by this node */
        union {
-               /** The affinity of the column. */
+               /** The Type of the column. */

> 
>> -		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.

See explanation above.

> 
>> + *
>> + * 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.

Fixed:

diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index fe5c56c07..33b860f36 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -392,7 +392,6 @@ codeApplyAffinity(Parse * pParse, int base, int n, char *zAff)
                base++;
                zAff++;
        }
-
        while (n > 1 && zAff[n - 1] == AFFINITY_BLOB) {
                n--;
        }