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

[Nikita Pettik <korablev@tarantool.org>]

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.

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