Re: [tarantool-patches] [PATCH v1 1/1] box: wrong is_nullable for multiple indexes

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

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.