[tarantool-patches] Re: [PATCH] Allow nullability mismatch in space indices and format.

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

Hi! Thanks for the patch! See 8 comments below.

1. Please, prefix the commit title with a subsystem name to
which the patch is related. (Here it is 'box: ' I think).

> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 2e19d2e3c..a640c23d3 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -84,12 +84,11 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
>   			if (part->fieldno >= field_count) {
>   				field->is_nullable = part->is_nullable;
>   			} else if (field->is_nullable != part->is_nullable) {
> -				diag_set(ClientError, ER_NULLABLE_MISMATCH,
> -					 part->fieldno + TUPLE_INDEX_BASE,
> -					 field->is_nullable ? "nullable" :
> -					 "not nullable", part->is_nullable ?
> -					 "nullable" : "not nullable");
> -				return -1;
> +				/*

2. On the branch here a leading white space is. Please, remove.

3. Please, wrap the comment by 66 symbols.

4. ER_NULLABLE_MISMATCH is now unused. Please, replace it with
a placeholder in errcode.h file. (For examples grep ER_UNUSED in
the file).

> +				 * In case of mismatch set the most strict
> +				 * option for is_nullable.
> +				 */
> +				field->is_nullable = false;
>   			}
>   
>   			/*
> diff --git a/test/engine/ddl.result b/test/engine/ddl.result
> index 0cb9ed6c2..c8fd8e6f2 100644
> --- a/test/engine/ddl.result
> +++ b/test/engine/ddl.result
> @@ -1431,9 +1431,12 @@ s:create_index('primary', { parts = { 'field1' } })
>   ---
>   - error: Primary index of the space 'test' can not contain nullable parts
>   ...
> -s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
> +-- this is allowed, but the actual part is_nullable stays false

5. Please, in comments start sentences from a capital letter and finish
with the dot.

> +pk = s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
> +---
> +...
> +pk:drop()
>   ---
> -- error: Field 1 is nullable in space format, but not nullable in index parts
>   ...
>   format[1].is_nullable = false
>   ---
> diff --git a/test/engine/null.result b/test/engine/null.result
> index 4abf85021..e0ad7df8e 100644
> --- a/test/engine/null.result
> +++ b/test/engine/null.result
> @@ -70,48 +70,8 @@ ok
>   ---
>   - false
>   ...
> --- Conflict of is_nullable in format and in parts.
> -parts[1].is_nullable = false
> ----
> -...
>   sk = s:create_index('sk', { parts = parts }) -- Fail.

6. Now the comment is wrong - this line does not fail.

>   ---
> -- error: Field 2 is nullable in space format, but not nullable in index parts
> -...
> --- Try skip nullable in format and specify in part.
> -parts[1].is_nullable = true
> ----
> -...
> -sk = s:create_index('sk', { parts = parts }) -- Ok.
> ----
> -...
> -format[2].is_nullable = nil
> ----
> -...
> -s:format(format) -- Fail.
> ----
> -- error: Field 2 is not nullable in space format, but nullable in index parts
> -...
> -sk:drop()
> ----
> -...
> --- Try to set nullable in part with no format.
> -s:format({})
> ----
> -...
> -sk = s:create_index('sk', { parts = parts })
> ----
> -...
> --- And then set format with no nullable.
> -s:format(format) -- Fail.
> ----
> -- error: Field 2 is not nullable in space format, but nullable in index parts
> -...
> -format[2].is_nullable = true
> ----
> -...
> -s:format(format) -- Ok.
> ----
>   ...
>   -- Test insert.
>   s:insert{1, 1}
> @@ -1560,3 +1520,168 @@ i2:select{}
>   s:drop()
>   ---
>   ...
> +-- gh-3430 allow different nullability in space format and indices.
> +-- resulting field nullability is the strictest of the two.
> +s = box.schema.space.create('test', {engine=engine})
> +---
> +...
> +pk = s:create_index('primary', {parts={1, 'unsigned'}})
> +---
> +...
> +sk = s:create_index('secondary', {parts={2, 'unsigned', is_nullable=false}})
> +---
> +...
> +format = {}
> +---
> +...
> +format[1] = {name = 'first', type = 'unsigned', is_nullable = false}
> +---
> +...
> +format[2] = {name = 'second', type = 'unsigned', is_nullable = false}
> +---
> +...
> +s:format(format)
> +---
> +...
> +-- field 2 is not nullable
> +s:insert{5}
> +---
> +- error: Tuple field count 1 is less than required by space format or defined indexes
> +    (expected at least 2)
> +...
> +s:insert{5, nil}

7. {5, nil} == {5}, so I think you can omit this insertion. It is
covered by the previous one.

8. What if I insert NULL into the space and then set this field to
be not nullable in format or one of indexes? Please, add a test.