From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 A882D42EF5C for ; Mon, 22 Jun 2020 23:47:27 +0300 (MSK) References: <20200622082321.GA30686@tarantool.org> From: Vladislav Shpilevoy Message-ID: <9e7bf285-b49c-6104-088f-02bf9c0b6266@tarantool.org> Date: Mon, 22 Jun 2020 22:47:25 +0200 MIME-Version: 1.0 In-Reply-To: <20200622082321.GA30686@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 , imeevma@tarantool.org Cc: tarantool-patches@dev.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_ 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); >> +} >> +