From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Subject: Re: [tarantool-patches] [PATCH v1 1/1] box: wrong is_nullable for multiple indexes References: From: Vladislav Shpilevoy Message-ID: Date: Fri, 2 Nov 2018 13:29:19 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit To: tarantool-patches@freelists.org, imeevma@tarantool.org, Vladimir Davydov List-ID: Hi! Thanks for the patch! On 01/11/2018 16:50, imeevma@tarantool.org wrote: > If field isn't defined by space format, than in case of multiple > indexes field option is_nullable was the same as it was for last > index that defines it. This is wrong as it should be 'true' only > if it is 'true' for all indexes that defines it. > > Closes #3744. > --- > Issue: https://github.com/tarantool/tarantool/issues/3744 > Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3744-wrong-is_nullable-for-multiple-indexes > > src/box/tuple_format.c | 12 ++---------- > test/engine/null.result | 24 ++++++++++++++++++++++++ > test/engine/null.test.lua | 9 +++++++++ > 3 files changed, 35 insertions(+), 10 deletions(-) > > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index 5f4899d..d4af54c 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -38,7 +38,7 @@ static intptr_t recycled_format_ids = FORMAT_ID_NIL; > static uint32_t formats_size = 0, formats_capacity = 0; > > static const struct tuple_field tuple_field_default = { > - FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, false, > + FIELD_TYPE_ANY, TUPLE_OFFSET_SLOT_NIL, false, true, > }; > > /** > @@ -81,15 +81,7 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys, > assert(part->fieldno < format->field_count); > struct tuple_field *field = > &format->fields[part->fieldno]; > - if (part->fieldno >= field_count) { > - field->is_nullable = part->is_nullable; > - } else if (field->is_nullable != part->is_nullable) { > - /* > - * In case of mismatch set the most > - * strict option for is_nullable. > - */ > - field->is_nullable = false; > - } > + field->is_nullable &= part->is_nullable; I have changed it to field->is_nullable = field->is_nullable && part->is_nullable; because it is rather logical operation (&&) than bitwise (&). I've force pushed the fix. Other part of the patch LGTM. Vova, please, review. If you will push it, be careful during merge into 2.1 - field->nullable_action appears there and it may have the same bug.