From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 2E040441840 for ; Mon, 30 Mar 2020 14:38:07 +0300 (MSK) Date: Mon, 30 Mar 2020 11:38:06 +0000 From: Nikita Pettik Message-ID: <20200330113805.GA18954@tarantool.org> References: <9a402676dbab6bdc4b1cbbdfc1f5069214fdfa06.1584707598.git.imeevma@gmail.com> <876090db-cb61-0c46-427a-d5a856fad4f1@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <876090db-cb61-0c46-427a-d5a856fad4f1@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove implicit cast for COMPARISON 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 30 Mar 00:29, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! > > See 15 comments below. > > I don't think I understood all the changed (because I am very out of > context), so I will revisited many of them when you send v2. > > On 20/03/2020 13:34, imeevma@tarantool.org wrote: > > This patch removes implicit cast for comparison. Also, it make > > search using an index work the same way as in case of fullscan in > > most cases. > > > > Closes #4230 > > Closes #4783 > > --- > > https://github.com/tarantool/tarantool/issues/4230 > > https://github.com/tarantool/tarantool/issues/4783 > > https://github.com/tarantool/tarantool/tree/imeevma/gh-4230-remove-implicit-cast-for-index-search-second > > > > @ChangeLog Remove implicit cast for comparison gh-4230 > > 1. Worth adding more details. What implicit cast? All of them? > > Do we need a docbot request for this? We definitely do need a docbot request. What is more, I'd explicitly agree all user-visible changes introduced in this patch with Peter Gulutzan. If Peter is already OK with it, Mergen, could you please copy link to the corresponding mail (cause I've a bit lost in all 'cast' threads). > > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > > index 1579cc9..cd626bd 100644 > > --- a/src/box/sql/sqlInt.h > > +++ b/src/box/sql/sqlInt.h > > @@ -1304,6 +1304,9 @@ enum trim_side_mask { > > (X) == FIELD_TYPE_UNSIGNED || \ > > (X) == FIELD_TYPE_DOUBLE) > > > > +#define sql_mp_type_is_numeric(X) ((X) == MP_INT || (X) == MP_UINT || \ > > + (X) == MP_FLOAT || (X) == MP_DOUBLE) > > > 7. Is there a ticket to stop conveting MEM_* to MP_*? Hm, I thought I created once such ticket, but in fact failed to find it. If you are OK with suggestion below concerning splitting mem type into two members, I'll open an issue. > MP_* now contains all MEM_* and even more, right? Not exactly. There are also VDBE specific MEMs like MEM_Frame, MEM_Cleared, MEM_TypeMask etc, and auxiliary MEMs - MEM_Dyn, MEM_Static, MEM_Ephem (i.e. allocation policy), MEM_Subtype, MEM_Agg and so forth. I guess still we can split struct mem->type into mp_type and aux_flags.