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

[Vladislav Shpilevoy]

Thanks for the fixes!

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

Are you sure? Looks like it is not renamed. Also, the comment
was about sql_index_type_is_ok and its parameter idx_type, but you
changed comparisonAffinity function.

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

I've already acked the patch. Please, hurry up Kirill Y. to push it,
rebase this branch, and replace ANY with SCALAR.

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

Vice versa? Not SCALAR with ANY, but ANY with SCALAR, I think.

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

See 5 comments below.

> -/*
> - * 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 Expr *p, enum field_type type)

1. const Expr -> const struct Expr

> -int sqlite3ExprNeedsNoAffinityChange(const Expr *, char);
> +
> +/**
> + * 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_no_type_change(const Expr *epr, enum field_type type);

2. epr -> p, and the same as 1.

> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
> index cd71641b0..d12a2a833 100644
> --- a/src/box/sql/vdbemem.c
> +++ b/src/box/sql/vdbemem.c
> @@ -1316,8 +1315,8 @@ valueFromExpr(sqlite3 * db,	/* The database connection */
>  			sqlite3ValueSetStr(pVal, -1, zVal, SQLITE_DYNAMIC);
>  		}
>  		if ((op == TK_INTEGER || op == TK_FLOAT)
> -		    && affinity == AFFINITY_BLOB) {
> -			sqlite3ValueApplyAffinity(pVal, AFFINITY_REAL);
> +		    && affinity == FIELD_TYPE_SCALAR) {
> +			sqlite3ValueApplyAffinity(pVal, FIELD_TYPE_INTEGER);

3. Why before your patch it was REAL and now it is INTEGER?

>  		} else {
>  			sqlite3ValueApplyAffinity(pVal, affinity);
>  		}
> diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
> index efbc91cf8..33b860f36 100644
> --- a/src/box/sql/wherecode.c
> +++ b/src/box/sql/wherecode.c
> @@ -423,10 +423,11 @@ updateRangeAffinityStr(Expr * pRight,	/* RHS of comparison */
>  {
>  	int i;
>  	for (i = 0; i < n; i++) {
> +		enum field_type type = sql_affinity_to_field_type(zAff[i]);
>  		Expr *p = sqlite3VectorFieldSubexpr(pRight, i);
> -		enum affinity_type aff = sqlite3ExprAffinity(p);
> -		if (sql_affinity_result(aff, zAff[i]) == AFFINITY_BLOB
> -		    || sqlite3ExprNeedsNoAffinityChange(p, zAff[i])) {
> +		enum field_type expr_type = sql_expr_type(p);
> +		if (sql_type_result(expr_type, type) == FIELD_TYPE_SCALAR ||
> +			sql_expr_needs_no_type_change(p, type)) {
>  			zAff[i] = AFFINITY_BLOB;
>  		}
>  	}

4. Something is wrong with formatting. sql_expr_needs_no_type_change should be
aligned under sql_type_result as a part of the condition.

> @@ -1166,20 +1167,12 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
>  		}
>  		struct index_def *idx_pk = space->index[0]->def;
>  		int fieldno = idx_pk->key_def->parts[0].fieldno;
> -		char affinity = is_format_set ?
> -				space->def->fields[fieldno].affinity :
> -				AFFINITY_BLOB;
> -		if (affinity == AFFINITY_UNDEFINED) {
> -			if (idx_pk->key_def->part_count == 1 &&
> -			    space->def->fields[fieldno].type ==
> -			    FIELD_TYPE_INTEGER)
> -				affinity = AFFINITY_INTEGER;
> -			else
> -				affinity = AFFINITY_BLOB;
> -		}
> +		char fd_type = is_format_set ?
> +				space->def->fields[fieldno].type :
> +				FIELD_TYPE_ANY;
>  

5. Why char? And the alignment is slightly wrong. Non-first lines
should be aligned under is_format_set.