[tarantool-patches] Re: [PATCH 6/6] sql: allow to specify UNSIGNED column type

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi!

Thanks for the fixes!

>> -------------------------
>> vdbe.c:307
>>
>>> 	case FIELD_TYPE_INTEGER:
>>> 	case FIELD_TYPE_UNSIGNED:
>>> 		if ((record->flags & MEM_Int) == MEM_Int)
>>> 			return 0;
>>> 		if ((record->flags & MEM_UInt) == MEM_UInt)
>>> 			return 0;
>>> 		if ((record->flags & MEM_Real) == MEM_Real) {
>>> 			int64_t i = (int64_t) record->u.r;
>>> 			if (i == record->u.r)
>>> 				mem_set_int(record, record->u.r,
>>> 					    record->u.r <= -1);
>>> 			return 0;
>>> 		}
>>
>> It is a part of function mem_apply_type. When target type is
>> UNSIGNED, and a value is MEM_Int, you do nothing. Why? Looks like
>> it is possible to pass here a negative value, and CAST UNSIGNED
>> would do nothing.
> 
> Basically, this function implements sort of implicit cast
> which takes place before comparison/assignment.
> For comparisons it makes no sense - we can compare
> integer with unsigned value - the latter is always greater.
> For assignment it is also meaningless: if we attempt
> at inserting negative values to unsigned field appropriate
> error will be raised anyway. If you can come up with
> specific example, let’s discuss it.
> 

I can't provide a test. But the function is named mem_apply_type,
and it doesn't apply type, when it is unsigned, and a value is
negative. Doesn't it look wrong to you?

If some code wants to get an integer, it can apply FIELD_TYPE_INTEGER
instead of FIELD_TYPE_UNSIGNED. IMO, an attempt to apply unsigned
to int should raise an error here. Otherwise this function can't
be named 'apply_type' because it ignores negative -> unsigned case.