Re: [Tarantool-patches] [PATCH v2 1/7] sql: remove implicit cast for assignment

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

>> +{
>> +	if ((mem->flags & MEM_Null) != 0)
>> +		return 0;
>> +	assert(type < field_type_MAX);
>> +	uint32_t flags = mem->flags;
>> +	switch (type) {
> 
> Instead of such long switch-cases could we organize it in one table
> containing valid conversions? I mean sort of field_mp_type_is_compatible()
> To provide not only check but execution mechanism you can fill
> table with pointers to functions implementing particular casts.

Better to do that when MEM_<type> flags will be replaced by field_type.
In that case you will be able to use the existing compatibility checkers
(as I hope).

>> +	case FIELD_TYPE_INTEGER:
>> +		if ((flags & (MEM_Int | MEM_UInt)) != 0)
>> +			return 0;
>> +		if ((flags & MEM_Real) == 0)
>> +			return -1;
>> +		double d = mem->u.r;
>> +		if (d >= (double)UINT64_MAX || d < (double)INT64_MIN)
>> +			return -1;
>> +		if (d < (double)INT64_MAX) {
>> +			int64_t i = (int64_t) d;
>> +			if (i == d) {
>> +				mem_set_int(mem, i, i <= -1);
>> +				return 0;
>> +			}
>> +			return -1;
>> +		}
>> +		uint64_t u = (uint64_t) d;
>> +		if (u == d) {
>> +			mem_set_u64(mem, u);
>> +			return 0;
>> +		}
>> +		return -1;
>> +	case FIELD_TYPE_SCALAR:
>> +		/* Can't cast MAP and ARRAY to scalar types. */
> 
> Except for map and arrays we alread have decimal. IS this function
> able to handle it?

It returns -1. Meaning it can't convert. Since DECIMAL is not yet
supported in SQL. So it should work and give an error, but yeah, a
test would be good to have.

>> +		if ((flags & MEM_Subtype) == 0 ||
>> +		    mem->subtype != SQL_SUBTYPE_MSGPACK)
>> +			return 0;
>> +		assert(mp_typeof(*mem->z) == MP_MAP ||
>> +		       mp_typeof(*mem->z) == MP_ARRAY);
>> +		return -1;
>> @@ -2776,6 +2883,31 @@ case OP_ApplyType: {
>>  	break;
>>  }
>>  
>> +/* Opcode: CheckType P1 P2 * P4 *
> 
> ApplyType was quite suitable name, meanwhile CheckType is a bit confusing
> since in fact it doesn't only check but cast (apply, coerce or whatever)
> mem to given type.

How about OP_SafeTypeCast? Or OP_SafeApplyType? Or OP_ImplicitCast?

>> + * Synopsis: type(r[P1@P2])
>> + *
>> + * Check that types of P2 registers starting from register
>> + * P1 are compatible with given with given field types in P4.
>> + */
>> +case OP_CheckType: {
>> +	enum field_type *types = pOp->p4.types;
>> +	assert(types != NULL);
>> +	assert(types[pOp->p2] == field_type_MAX);
>> +	pIn1 = &aMem[pOp->p1];
>> +	enum field_type type;
>> +	while((type = *(types++)) != field_type_MAX) {
>> +		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
>> +		assert(memIsValid(pIn1));
>> +		if (mem_check_types(pIn1, type) != 0) {
>> +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
>> +				 mem_type_to_str(pIn1), field_type_strs[type]);
>> +			goto abort_due_to_error;
>> +		}
>> +		pIn1++;
>> +	}
>> +	break;
>> +}
>> +
>> diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
>> index 8dad2db9a..9e8586ffc 100644
>> --- a/src/box/sql/vdbemem.c
>> +++ b/src/box/sql/vdbemem.c
>> @@ -839,6 +839,13 @@ mem_set_int(struct Mem *mem, int64_t value, bool is_neg)
>>  	}
>>  }
>>  
>> +void
>> +mem_set_double(struct Mem *mem, double value)
>> +{
> 
> I see inconsistency with other setters: they provide auxiliary
> clean-up in case mem has one of Agg/Dyn/Frame flags. Please
> investigate whether it is really required and if it is so add
> it to current one (or remove from other setters).

+. I believe we forgot that the mem could contain some dynamically
allocated things on the heap.

Btw, this function could be added in a separate commit probably. And
applied to all the existing places were we set the double type manually.

>> +	mem->u.r = value;
>> +	MemSetTypeFlag(mem, MEM_Real);
>> +}
>> +