[tarantool-patches] Re: [PATCH] Allow nullability mismatch in space indices and format.
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Aug 8 15:50:26 MSK 2018
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.
More information about the Tarantool-patches
mailing list