Re: [Tarantool-patches] [PATCH 13/14] WIP: module api: expose box_tuple_extract_key_ex()

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

Or instead of _ex you could make it a method of key_def and
call it box_key_def_extract_key(...). IMO that would be more
appropriate as a key_def method.

On 23.09.2020 03:14, Alexander Turenko wrote:
> Unlike box_tuple_extract_key() it accepts a key_def structure, not
> space_id, index_id pair.
> 
> Another difference from box_tuple_extract_key() is that this function
> allows to pass so called multikey index. See commit 2.2.0-259-gf1d9f2575
> ('box: introduce multikey indexes in memtx') for details.

I hate multikeys even more now.

See 2 comments below.

> Note: The <multikey_idx> parameter is ignored on the backported version
> of the patch on 1.10. Also there is no <MULTIKEY_NONE> constant in the
> backport.
> 
> XXX: Add a module API test.
> 
> Part of #5273
> diff --git a/src/box/field_map.h b/src/box/field_map.h
> index d8ef726a1..6e3c5be25 100644
> --- a/src/box/field_map.h
> +++ b/src/box/field_map.h
> @@ -38,12 +38,16 @@
>  struct region;
>  struct field_map_builder_slot;
>  
> +/** \cond public */
> +
>  /**
>   * A special value of multikey index that means that the key
>   * definition is not multikey and no indirection is expected.
>   */
>  enum { MULTIKEY_NONE = -1 };

1. Why didn't you add a new BOX_KEY_DEF_MULTIKEY_NONE constant?
I thought we don't want to expose any internal definitions.

Also you could specify in the comment that pass -1 if don't want
to use multikey. Without a enum. I think it would be totally
fine.

> diff --git a/src/box/key_def_api.h b/src/box/key_def_api.h
> index 0cef7d1ea..28fcc32da 100644
> --- a/src/box/key_def_api.h
> +++ b/src/box/key_def_api.h
> @@ -243,6 +243,26 @@ box_tuple_compare_with_key(box_tuple_t *tuple_a, const char *key_b,
>  API_EXPORT box_key_def_t *
>  box_key_def_merge(const box_key_def_t *first, const box_key_def_t *second);
>  
> +/**
> + * Extract key from tuple by given key definition and return
> + * buffer allocated on the box region with this key.
> + * @sa <box_region_truncate>().
> + *
> + * This function has O(n) complexity, where n is the number of key
> + * parts.
> + *
> + * @param tuple - tuple from which need to extract key

2. Lets be consistent and not use '-' between '@param name'
and the parameter description. In addition to using capital
letter in the beginning and dots in the end. Sorry for OCD.

> + * @param key_def - definition of key that need to extract
> + * @param multikey_idx - multikey index hint
> + * @param key_size_ptr - here will be size of extracted key
> + *
> + * @retval not NULL Success
> + * @retval NULL     Memory allocation error
> + */
> +API_EXPORT char *
> +box_tuple_extract_key_ex(box_tuple_t *tuple, box_key_def_t *key_def,
> +			 int multikey_idx, uint32_t *key_size_ptr);
> +
>  /** \endcond public */