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

[Mergen Imeev <imeevma@tarantool.org>]

On 23.06.2020 00:08, Nikita Pettik wrote:
> On 22 Jun 22:47, Vladislav Shpilevoy wrote:
>>>> +{
>>>> +	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.
> I'm afraid it may take a while..Surely, I don't insisnt on this
> refactoring, patch can be pushed without it. Anyway it would be
> nice to see it.

I will try to fix it.


>> In that case you will be able to use the existing compatibility checkers
>> (as I hope).
>>
>>>> +		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?
> I don't understand why it is supposed to be 'safe'..
> Any of suggested names is way much better than CheckType, but
> still I see no reason to avoid using original 'ApplyType'.
I can replace OP_CheckType with OP_ApplyType after removing the
original OP_ApplyType, but I think OP_ImplicitCast is better. Do
you mind if I use this name?

>   
>>>> + * 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.
> Agree.

I will fix it.