[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!

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

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

>          return pExpr->type;
>>>> 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).
>>
>> Despite our ability to select complex types, in SQL they are all
>> mere binary data with *sub*type msgpack, not array/map type. So
>> in SQL we never truly work with anything but SCALAR.
> 
> Wait, at the stage of query compiling we don’t operate on
> any subtypes. We simply fetch space_def->fields->type,
> which may turn out to be FIELD_TYPE_MAP/ARRAY or
> whatever.

Hmm, yes, you are right. We should consider array and map too.

See 4 comments below.

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

> +	case TK_CASE:
> +		assert(pExpr->x.pList->nExpr >= 2);
> +		/*
> +		 * CASE expression comes at least with one
> +		 * WHEN and one THEN clauses. So, first
> +		 * expression always represents WHEN
> +		 * argument, and the second one - THEN.
> +		 *
> +		 * TODO: We must ensure that all THEN clauses
> +		 * have arguments of the same type.
> +		 */
> +		return sql_expr_type(pExpr->x.pList->a[1].pExpr);
>  	case TK_LT:
>  	case TK_GT:
>  	case TK_EQ:
> @@ -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.

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

> -	case AFFINITY_TEXT:
> -		return idx_affinity == AFFINITY_TEXT;
> +	case FIELD_TYPE_STRING:
> +		return field_type == FIELD_TYPE_STRING;
>  	default:
> -		return sqlite3IsNumericAffinity(idx_affinity);
> +		return sql_type_is_numeric(field_type);
>  	}
>  }
>  
> @@ -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.

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