From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 2B1A04429E1 for ; Tue, 23 Jun 2020 09:44:12 +0300 (MSK) References: <20200622082321.GA30686@tarantool.org> <9e7bf285-b49c-6104-088f-02bf9c0b6266@tarantool.org> <20200622210825.GB32744@tarantool.org> From: Mergen Imeev Message-ID: <6dc7eec1-27e8-b2fe-4a7b-981ad6e8aaaf@tarantool.org> Date: Tue, 23 Jun 2020 09:44:10 +0300 MIME-Version: 1.0 In-Reply-To: <20200622210825.GB32744@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH v2 1/7] sql: remove implicit cast for assignment List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , Vladislav Shpilevoy Cc: tarantool-patches@dev.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_ 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.