From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 61A6442EF5C for ; Tue, 23 Jun 2020 00:08:26 +0300 (MSK) Date: Mon, 22 Jun 2020 21:08:25 +0000 From: Nikita Pettik Message-ID: <20200622210825.GB32744@tarantool.org> References: <20200622082321.GA30686@tarantool.org> <9e7bf285-b49c-6104-088f-02bf9c0b6266@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <9e7bf285-b49c-6104-088f-02bf9c0b6266@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.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_ 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.