From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (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 454B54696C3 for ; Sat, 11 Apr 2020 00:52:35 +0300 (MSK) Date: Sat, 11 Apr 2020 00:52:33 +0300 From: Mergen Imeev Message-ID: <20200410215232.GA5232@tarantool.org> References: <91166652f0b9cd151ca485538e6e895639c62acd.1585823952.git.imeevma@gmail.com> <20200410000445.GA14187@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: specify field types in ephemeral space format List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: tarantool-patches@dev.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.