[tarantool-patches] Re: [PATCH v1 1/1] sql: invalid integer type in arithmetic operations

[n.pettik]

> On 24 Apr 2019, at 22:56, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
>> index b3613d3ea..9b52e90f3 100644
>> --- a/src/box/sql/expr.c
>> +++ b/src/box/sql/expr.c
>> @@ -342,8 +342,19 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_explicit_coll, uint32_t *coll_id,
>> enum field_type
>> sql_type_result(enum field_type lhs, enum field_type rhs)
>> {
>> -	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs))
>> +	if (sql_type_is_numeric(lhs) || sql_type_is_numeric(rhs)) {
>> +		if (lhs == FIELD_TYPE_NUMBER || rhs == FIELD_TYPE_NUMBER)
>> +			return FIELD_TYPE_NUMBER;
>> +		if (lhs == FIELD_TYPE_INTEGER || rhs == FIELD_TYPE_INTEGER)
>> +			return FIELD_TYPE_INTEGER;
>> +		/*
>> +		 * FIXME: FIELD_TYPE_UNSIGNED static type is not
>> +		 * allowed yet.
>> +		 */
>> +		assert(lhs == FIELD_TYPE_UNSIGNED ||
>> +		       rhs == FIELD_TYPE_UNSIGNED);
> 
> How does it work? If it is not allowed, then lhs and rhs should not
> be equal to FIELD_TYPE_UNSIGNED, and this assertion should fail, it is not?
> 
> (I did not test, just looked at the diff in the mailing list)


Execution flaw hits this branch if both operands are numeric.
We consider only _INTEGER, _NUMBER and _UNSIGNED be
numeric types. If we hit this assertion, then both operands should
be _UNSIGNED (if I’m not mistaken, condition should be logical
AND not OR).

Surely, we still can create space and set format with unsigned types
from Lua, so strictly speaking UNSIGNED is allowed even now.
But we can’t set UNSIGNED as a type of column in SQL, and we don’t
set this type in meta. So in some sense it is not allowed.
Mb it is worth fixing comment. Or return _UNSIGNED instead
of _NUMBER in this case. I guess there will be no severe consequences.