From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 9 Apr 2018 23:39:25 +0300 From: Konstantin Osipov Subject: Re: [PATCH 06/12] space: space_vtab::build_secondary_key => build_index Message-ID: <20180409203925.GG4527@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [18/04/09 10:33]: > - space_build_secondary_key_xc(new_index_def->iid != 0 ? > - alter->new_space : alter->old_space, > - alter->new_space, new_index); > + space_build_index_xc(new_index_def->iid != 0 ? > + alter->new_space : alter->old_space, > + new_index, alter->new_space->format); > } This is confusing, why do you ever need to pass the old space? A new index should always be built in the new space, no? > /** > - * Called with the new empty secondary index. > - * Fill the new index with data from the primary > - * key of the space. > + * Build a new index, primary or secondary, and fill it > + * with tuples stored in the given space. The function is > + * supposed to assure that all tuples conform to the new > + * format. > */ > - int (*build_secondary_key)(struct space *old_space, > - struct space *new_space, > - struct index *new_index); > + int (*build_index)(struct space *space, struct index *index, > + struct tuple_format *format); Not having parameter 'space' documented, and not having the relationship between 'space' and 'format' documented (why can't we use space->format) doesn't provide more clarity either. -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov