Re: [Tarantool-patches] [PATCH v1 1/1] sql: remove implicit cast for COMPARISON

[Mergen Imeev <imeevma@tarantool.org>]

On Mon, Mar 30, 2020 at 11:38:06AM +0000, Nikita Pettik wrote:
> 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).
>
Ok, I will do all these in the next version of the patch.

> > > 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.
> 
Does anyony mind if I do all this here, before main patch-set?

> > 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.
>