From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Fri, 26 Jul 2019 15:15:36 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH v5 3/3] box: introduce func_index
Message-ID: <20190726121536.GA4659@esperanza>
References: <cover.1564134114.git.kshcherbatov@tarantool.org>
 <38c49e4a186f40139488ad743fb0c37e458d5a65.1564134114.git.kshcherbatov@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <38c49e4a186f40139488ad743fb0c37e458d5a65.1564134114.git.kshcherbatov@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org, kostja@tarantool.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Fri, Jul 26, 2019 at 12:43:39PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/index_def.h b/src/box/index_def.h
> index 6dac28377..1d60c982b 100644
> --- a/src/box/index_def.h
> +++ b/src/box/index_def.h
> @@ -298,6 +302,25 @@ index_def_update_optionality(struct index_def *def, uint32_t min_field_count)
>  	key_def_update_optionality(def->cmp_def, min_field_count);
>  }
>  
> +/**
> + * Update func pointer for a functional index key definition.
> + * @param def Index def, containing key definitions to update.
> + * @param func The func_index function pointer.
> + */
> +static inline void
> +index_def_set_func(struct index_def *def, struct func *func)
> +{
> +	assert(def->opts.func_id > 0 &&
> +	       def->key_def->for_func_index && def->key_def->for_func_index);

for_func_index && for_func_index

> +	/*
> +	 * Set func_index_func for functional index key
> +	 * definition. It is used in key_list module to extract
> +	 * a key for given tuple.
> +	 */
> +	def->key_def->func_index_func = func;
> +	def->cmp_def->func_index_func = NULL;

Hmm, why is cmp_def->func_index_func set to NULL?

> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index 02c1cb0ff..f570a1c08 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -885,11 +885,29 @@ local function upgrade_func_to_2_2_1()
>                                        collation = 'unicode_ci'}}})
>  end
>  
> +local function create_func_index()
> +    log.info("Create _func_index space")
> +    local _func_index = box.space[box.schema.FUNC_INDEX_ID]
> +    local _space = box.space._space
> +    local _index = box.space._index
> +    local format = {{name='space_id', type='unsigned'},
> +                    {name='index_id', type='unsigned'},
> +                    {name='func_id',  type='unsigned'}}
> +    _space:insert{_func_index.id, ADMIN, '_func_index', 'memtx', 0,
> +                  setmap({}), format}
> +    _index:insert{_func_index.id, 0, 'primary', 'tree', {unique = true},
> +                  {{0, 'unsigned'}, {1, 'unsigned'}, {2, 'unsigned'}}}

Why do we need to make all the three fields parts of the primary index?
Shouldn't space and index id be enough?

Other than that, the patch is fine by me.