Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for review. My answers below.

On Fri, Apr 10, 2020 at 09:16:28PM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> See 2 comments below.
> 
> > diff --git a/src/box/sql.c b/src/box/sql.c
> > index 27089bd..288c4fa 100644
> > --- a/src/box/sql.c
> > +++ b/src/box/sql.c
> > @@ -337,14 +337,15 @@ sql_ephemeral_space_create(uint32_t field_count, struct sql_key_info *key_info)
> >  	 * length of each name is no more than strlen("_COLUMN_")
> >  	 * + 5.
> >  	 */
> > -	assert(SQL_MAX_COLUMN < 9999);
> > +	assert(SQL_MAX_COLUMN <= 2000);
> >  	uint32_t name_len = sizeof("_COLUMN_") + 5;
> 
> 1. sizeof("_COLUMN_") != strlen("_COLUMN_"). The latter is 8,
> the former is 9. Better use strlen().
> 
Thanks, fixed.

> >  	uint32_t size = field_count * (sizeof(struct field_def) + name_len +
> >  				       sizeof(struct key_part_def));
> >  	struct field_def *fields = region_alloc(region, size);
> > -	struct key_part_def *parts = (void *)fields +
> > +	struct key_part_def *ephemer_key_parts = (void *)fields +
> >  				     field_count * sizeof(struct field_def);
> > -	char *names = (char *)parts + field_count * sizeof(struct key_part_def);
> > +	char *names = (char *)ephemer_key_parts +
> > +		      field_count * sizeof(struct key_part_def);
> >  	for (uint32_t i = 0; i < field_count; ++i) {
> >  		struct field_def *field = &fields[i];
> >  		field->name = names;
> 
> 2. It is worth extending the commit message with more details, how
> is it needed for 4256 and 4692, with all what we discussed in private.
> 
Done:

    sql: specify field types in ephemeral space format
    
    This patch allows to specify field types in ephemeral space format.
    Prior to this patch, all fields had a SCALAR field type.
    
    This patch allows us to not use the primary index to obtain field
    types, since now the ephemeral space has field types in the
    format. This allows us to change the structure of the primary
    index, which helps to solve the issue #4256. In addition, since we
    can now set the field types of the ephemeral space, we can use
    this feature to set the field types according to the left value of
    the IN operator. This will fix issue #4692.
    
    Needed for #4256
    Needed for #4692
    Closes #3841


> Please, do the things above and send to Nikita.
> 
> Also, in case it will be pushed in this form to master, I propose you to
> create a ticket on ephemeral_space API rework. As you fairly noticed, we
> have issues with using sql_key_info as, in fact, a format definition.
> We basically *can't* index fields not added to the format.
Ok, will do.