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

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

>>>>> 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
>>> 
>>> I've already acked the patch. Please, hurry up Kirill Y. to push it,
>>> rebase this branch, and replace ANY with SCALAR.
>> I had to add some other fixes to make this happen:
>> Changes to sql: replace affinity with field type for func:
>> diff --git a/test-run b/test-run
>> index 02207efd2..4c3f11812 160000
>> --- a/test-run
>> +++ b/test-run
>> @@ -1 +1 @@
>> -Subproject commit 02207efd2ca44067b76c79bf012f142016d929ae
>> +Subproject commit 4c3f11812c0c17f08780f31b6a748eef1126b2e2
> 
> 1. ???

Accidentally trapped to diff.

>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index fdf00273a..e1a0dfa73 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -92,16 +111,14 @@ sql_expr_type(struct Expr *pExpr)
>>                  * return INTEGER.
>>                  */
>>                 return FIELD_TYPE_INTEGER;
>> -       }
>> -       /*
>> -        * In case of unary plus we shouldn't discard
>> -        * type of operand (since plus always features
>> -        * NUMERIC type).
>> -        */
>> -       if (op == TK_UPLUS) {
>> +       case TK_UMINUS:
>> +       case TK_UPLUS:
>> +       case TK_NO:
>> +       case TK_BITNOT:
>>                 assert(pExpr->pRight == NULL);
>> -               return pExpr->pLeft->type;
>> +               return sql_expr_type(pExpr->pLeft);
>>         }
>> +
> 
> 2. Stray empty line.

Removed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e1a0dfa73..4ff7169ab 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -118,7 +118,6 @@ sql_expr_type(struct Expr *pExpr)
                assert(pExpr->pRight == NULL);
                return sql_expr_type(pExpr->pLeft);
        }
-
        return pExpr->type;
 }

>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index d263c1bb5..e1a0dfa73 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -81,27 +57,46 @@ sqlite3ExprAffinity(Expr * pExpr)
>> 	case TK_SELECT:
>> 		assert(pExpr->flags & EP_xIsSelect);
>> 		el = pExpr->x.pSelect->pEList;
>> -		return sqlite3ExprAffinity(el->a[0].pExpr);
>> +		return sql_expr_type(el->a[0].pExpr);
>> 	case TK_CAST:
>> 		assert(!ExprHasProperty(pExpr, EP_IntValue));
>> -		return pExpr->affinity;
>> +		return pExpr->type;
>> 	case TK_AGG_COLUMN:
>> 	case TK_COLUMN:
>> +	case TK_TRIGGER:
>> 		assert(pExpr->iColumn >= 0);
>> -		return sqlite3TableColumnAffinity(pExpr->space_def,
>> -						  pExpr->iColumn);
>> +		return pExpr->space_def->fields[pExpr->iColumn].type;
>> 	case TK_SELECT_COLUMN:
>> 		assert(pExpr->pLeft->flags & EP_xIsSelect);
>> 		el = pExpr->pLeft->x.pSelect->pEList;
>> -		return sqlite3ExprAffinity(el->a[pExpr->iColumn].pExpr);
>> +		return sql_expr_type(el->a[pExpr->iColumn].pExpr);
>> 	case TK_PLUS:
>> 	case TK_MINUS:
>> 	case TK_STAR:
>> 	case TK_SLASH:
>> +	case TK_REM:
>> +	case TK_BITAND:
>> +	case TK_BITOR:
>> +	case TK_LSHIFT:
>> +	case TK_RSHIFT:
>> 		assert(pExpr->pRight != NULL && pExpr->pLeft != NULL);
>> -		enum affinity_type lhs_aff = sqlite3ExprAffinity(pExpr->pLeft);
>> -		enum affinity_type rhs_aff = sqlite3ExprAffinity(pExpr->pRight);
>> -		return sql_affinity_result(rhs_aff, lhs_aff);
>> +		enum field_type lhs_type = sql_expr_type(pExpr->pLeft);
>> +		enum field_type rhs_type = sql_expr_type(pExpr->pRight);
>> +		return sql_type_result(rhs_type, lhs_type);
>> +	case TK_CONCAT:
>> +			return FIELD_TYPE_STRING;
> 
> 1. Indentation.

Fixed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e1a0dfa73..91d61b2d9 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -84,7 +84,7 @@ sql_expr_type(struct Expr *pExpr)
                enum field_type rhs_type = sql_expr_type(pExpr->pRight);
                return sql_type_result(rhs_type, lhs_type);
        case TK_CONCAT:
-                       return FIELD_TYPE_STRING;
+               return FIELD_TYPE_STRING;
        case TK_CASE:

>> @@ -252,74 +245,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)
>> +enum field_type
>> +sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type field_type)
> 
> 2. Returns 'bool true', but the return value type is enum field_type?
> Please, change to bool.

Fixed:

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index e1a0dfa73..2bc4f00cf 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -285,7 +284,7 @@ expr_cmp_mutual_type(struct Expr *pExpr)
  * @retval Return true if the index with @field_type may be used to
  * implement the comparison in expr.
  */
-enum field_type
+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 1;
+               return true;
        case FIELD_TYPE_STRING:
                return field_type == FIELD_TYPE_STRING;
        default:
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index d892631d8..8f75546de 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -4284,7 +4284,7 @@ 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);
 
-enum field_type
+bool
 sql_expr_cmp_type_is_compatible(struct Expr *expr, enum field_type idx_type);

>> {
>> -	char aff = comparisonAffinity(pExpr);
>> -	switch (aff) {
>> -	case AFFINITY_BLOB:
>> +	enum field_type type = expr_cmp_mutual_type(expr);
>> +	switch (type) {
>> +	case FIELD_TYPE_SCALAR:
>> 		return 1;
> 
> 3. 1 -> true

Fixed, see above.

>> @@ -2140,21 +2116,11 @@ sqlite3ExprCanBeNull(const Expr * p)
>> 	}
>> }
>> -/*
>> - * 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
>> -sqlite3ExprNeedsNoAffinityChange(const Expr * p, char aff)
>> +sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
> 
> 4. The same. Anyway you changed the function name, so without
> increasing the diff you can change its return value to bool,
> and the values below 0 -> false, 1 -> true.

That’s true. Fixed:

@@ -2116,12 +2115,12 @@ sqlite3ExprCanBeNull(const Expr * p)
        }
 }
 
-int
+bool
 sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
 {
        u8 op;
        if (type == FIELD_TYPE_SCALAR)
-               return 1;
+               return true;
        while (p->op == TK_UPLUS || p->op == TK_UMINUS) {
                p = p->pLeft;
        }
@@ -2136,13 +2135,13 @@ sql_expr_needs_no_type_change(const struct Expr *p, enum field_type type)
        case TK_STRING:
                return type == FIELD_TYPE_STRING;
        case TK_BLOB:
-               return 1;
+               return true;
        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:
-               return 0;
+               return false;
        }
 }

index d892631d8..f200752ce 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3734,7 +3734,7 @@ int sqlite3ExprCanBeNull(const Expr *);
  * is harmless. A false positive, however, can result in the wrong
  * answer.
  */
-int
+bool
 sql_expr_needs_no_type_change(const struct Expr *expr, enum field_type type);