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

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

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

Well, that is actually true. See below, I removed this function at all.

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

I guess because expr_cmp_mutal_type returns SCALAR or NUMBER
as you noticed. The first one is compatible with all scalar types and not
vice versa. So ..._type2(expr_cmp_mutual_type(expr), field_type) is not
the same as …_type2(field_type, expr_cmp_mutual_type(expr));

I’ve made comparative table and …_type2(expr_cmp_mutal_type, field_type)
gives us the same results as sql_expr_type_is_compatible.
Hence, I will remove this function and use …_type2() instead.
Diff:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index db8577d1c..36da1ecdb 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -252,12 +252,7 @@ sql_type_result(enum field_type lhs, enum field_type rhs)
        return FIELD_TYPE_SCALAR;
 }
 
-/**
- * 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
+enum field_type
 expr_cmp_mutual_type(struct Expr *pExpr)
 {
        assert(pExpr->op == TK_EQ || pExpr->op == TK_IN || pExpr->op == TK_LT ||
@@ -278,25 +273,6 @@ expr_cmp_mutual_type(struct Expr *pExpr)
        return type;
 }
 
-/**
- * @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.
- */
-bool
-sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)
-{
-       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;
-       default:
-               return sql_type_is_numeric(field_type);
-       }
-}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 047a77b68..61f316550 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4284,8 +4284,13 @@ sql_emit_table_types(struct Vdbe *v, struct space_def *def, int reg);
 enum field_type
 sql_type_result(enum field_type lhs, enum field_type rhs);
 
-bool
-sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type idx_type);
+/**
+ * pExpr is a comparison operator. Return the type affinity
+ * that should be applied to both operands prior to doing
+ * the comparison.
+ */
+enum field_type
+expr_cmp_mutual_type(struct Expr *pExpr);
 diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 219009ef8..6a47f8c99 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -298,7 +298,9 @@ whereScanNext(WhereScan * pScan)
                                                /* Verify the affinity and collating sequence match */
                                                if ((pTerm->eOperator & WO_ISNULL) == 0) {
                                                        pX = pTerm->pExpr;
-                                                       if (!sql_expr_cmp_type_is_compatible(pX, pScan->idx_type))
+                                                       enum field_type expr_type =
+                                                               expr_cmp_mutual_type(pX);
+                                                       if (!field_type1_contains_type2(expr_type, pScan->idx_type))
                                                                continue;
                                                        if (pScan->is_column_seen) {
                                                                Parse *pParse =
@@ -690,7 +692,6 @@ termCanDriveIndex(WhereTerm * pTerm,        /* WHERE clause term to check */
                  Bitmask notReady      /* Tables in outer loops of the join */
     )
 {
-       char aff;
        if (pTerm->leftCursor != pSrc->iCursor)
                return 0;
        if ((pTerm->eOperator & WO_EQ) == 0)
@@ -699,8 +700,9 @@ termCanDriveIndex(WhereTerm * pTerm,        /* WHERE clause term to check */
                return 0;
        if (pTerm->u.leftColumn < 0)
                return 0;
-       aff = pSrc->pTab->aCol[pTerm->u.leftColumn].affinity;
-       if (!sql_expr_cmp_type_is_compatible(pTerm->pExpr, aff))
+       enum field_type type = pSrc->pTab->def->fields[pTerm->u.leftColumn].type;
+       enum field_type expr_type = expr_cmp_mutual_type(pTerm->pExpr);
+       if (!field_type1_contains_type2(expr_type, type))
                return 0;
        return 1;
 }

(The last change is related to dead code).

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

You are absolutely right, just wanted to make it look better
(indeed, not the best try). Returned back:

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;

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

diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 047a77b68..6e4ead573 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 Type of the column. */
+               /** The type of the column. */

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

@@ -4304,7 +4304,7 @@ sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type idx_type);
  * SELECT * FROM t1 WHERE (select a from t1);
  */
 enum field_type
-sql_expr_type(Expr *pExpr);
+sql_expr_type(struct Expr *pExpr);