[patches] [PATCH 4/4] Allow to do not specify tail nullable index columns
v.shpilevoy at tarantool.org
v.shpilevoy at tarantool.org
Sat Feb 17 23:38:57 MSK 2018
С уважением, Владислав Шпилевой.
> 17 февр. 2018 г., в 23:19, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
>
>> On Tue, Feb 13, 2018 at 06:28:38PM +0300, Vladislav Shpilevoy wrote:
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index c48e89f9e..cb3b8353d 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -323,7 +323,39 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
>> space->def->field_count) != 0)
>> diag_raise();
>> }
>> - key_def = key_def_new_with_parts(part_def, part_count);
>> + struct space_def *sd = space->def;
>> + int unchanged_index_count = 0;
>> + struct key_def **keys = NULL;
>> + /*
>> + * To detect which key parts are optional, min_field_count
>> + * is required. But min_field_count from the old space
>> + * format can not be used. For example, consider the case,
>> + * when a space has no format, has a primary index on the
>> + * first field and has a single secondary index on a
>> + * non-nullable second field. Min field count here is 2.
>> + * Now alter the secondary index to make its part be
>> + * nullable. The 'space' variable here is the old space,
>> + * where min_field_count is still 2, but actually it is
>> + * already 1. Actual min_field_count must be calculated
>> + * using old unchanged indexes, NEW definition of an
>> + * updated index and a space format, defined by a user.
>> + */
>> + for (uint32_t i = 0; i < space->index_count; ++i) {
>> + if (space->index[i]->def->iid != index_id)
>> + ++unchanged_index_count;
>> + }
>> + if (unchanged_index_count > 0) {
>> + size_t bsize = unchanged_index_count * sizeof(keys[0]);
>> + keys = (struct key_def **) region_alloc_xc(&fiber()->gc, bsize);
>> + for (uint32_t i = 0, j = 0; i < space->index_count; ++i) {
>> + if (space->index[i]->def->iid != index_id)
>> + keys[j++] = space->index[i]->def->key_def;
>> + }
>> + }
>> + uint32_t min_field_count =
>> + tuple_format_min_field_count(keys, unchanged_index_count,
>> + sd->fields, sd->field_count);
>> + key_def = key_def_new_with_parts(part_def, part_count, min_field_count);
>
> What will happen if there are two indexes over the same field, nullable
> and not nullable, and the one which is not nullable is set to be
> nullable? I don't see where you update is_optional then.
Is_optional in such a case is set due to decreased min_field_count.
>
> AFAIU is_optional should be updated for all indexes in alter_space_do()
> or in AlterSpaceOps, not here.
I disagree.
>
>> diff --git a/src/box/key_def.h b/src/box/key_def.h
>> index 96f4cdbf9..a7d6eedfb 100644
>> --- a/src/box/key_def.h
>> +++ b/src/box/key_def.h
>> @@ -72,6 +72,8 @@ struct key_part {
>> struct coll *coll;
>> /** True if a part can store NULLs. */
>> bool is_nullable;
>> + /** True, if a part can absense in a tuple. */
>
> 'absence' is always a noun, never a verb.
> Fix your comments please.
Ok.
>
>> + bool is_optional;
>> };
>>
>> struct key_def;
>> @@ -126,6 +128,11 @@ struct key_def {
>> uint32_t unique_part_count;
>> /** True, if at least one part can store NULL. */
>> bool is_nullable;
>> + /**
>> + * True, if some key parts can absense in a tuple. These
>> + * fields assumed to be MP_NIL.
>> + */
>> + bool is_optional;
>
> You add is_optional to both key_def and key_part, but I fail to
> understand why you need them. You can detect if a field is missing
> in a tuple without any flags, right? So why do you need them?
It is Kostja’s decision.
>
>> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
>> index dcfb4f636..10c00a6f4 100644
>> --- a/src/box/tuple_compare.cc
>> +++ b/src/box/tuple_compare.cc
>> @@ -589,18 +616,12 @@ tuple_compare_with_key_slowpath(const struct tuple *tuple, const char *key,
>> return 0;
>> }
>>
>> -/**
>> - * is_sequential_nullable_tuples used to determine if this
>> - * function is used to compare two tuples, which keys are
>> - * sequential. In such a case it is necessary to compare them
>> - * using primary parts if a NULL was met.
>> - */
>> -template<bool is_nullable, bool is_sequential_nullable_tuples>
>> +template<bool is_nullable>
>> static inline int
>> key_compare_parts(const char *key_a, const char *key_b, uint32_t part_count,
>> const struct key_def *key_def)
>
> To ease review, removal of is_sequential_nullable_tuples template
> parameter should be done in a separate patch.
>
>> -static int
>> -tuple_compare_sequential_nullable(const struct tuple *tuple_a,
>> - const struct tuple *tuple_b,
>> - const struct key_def *key_def)
>
> Turning tuple_compare_sequential_nullable into a template parameter
> should be done in a separate patch. Folding it in the patch removing
> is_sequential_nullable_tuples (see above) would be OK.
More information about the Tarantool-patches
mailing list