[tarantool-patches] Re: [PATCH] Allow nullability mismatch in space indices and format.
Sergey Petrenko
sergepetrenko at tarantool.org
Thu Aug 9 10:09:49 MSK 2018
Hi! Thank you for review. I fixed your comments.
Please find the new diff below.
> 8 авг. 2018 г., в 15:50, Vladislav Shpilevoy <v.shpilevoy at 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).
Done.
>
>> 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).
Done, done, done.
>> + * 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.
Done.
>
>> +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.
Fixed.
>
>> ---
>> -- 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.
Done.
> 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.
Added an extensive test, setting in format fails, setting in index fails.
Here’s diff:
src/box/errcode.h | 2 +-
src/box/tuple_format.c | 12 +--
test/box/misc.result | 5 +-
test/engine/ddl.result | 10 ++-
test/engine/ddl.test.lua | 6 +-
test/engine/null.result | 213 +++++++++++++++++++++++++++++++++++++---------
test/engine/null.test.lua | 78 +++++++++++++----
7 files changed, 255 insertions(+), 71 deletions(-)
diff --git a/src/box/errcode.h b/src/box/errcode.h
index 3d5f66af8..4115e6b65 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -205,7 +205,7 @@ struct errcode_record {
/*150 */_(ER_CANT_CREATE_COLLATION, "Failed to initialize collation: %s.") \
/*151 */_(ER_WRONG_COLLATION_OPTIONS, "Wrong collation options (field %u): %s") \
/*152 */_(ER_NULLABLE_PRIMARY, "Primary index of the space '%s' can not contain nullable parts") \
- /*153 */_(ER_NULLABLE_MISMATCH, "Field %d is %s in space format, but %s in index parts") \
+ /*153 */_(ER_UNUSED, "") \
/*154 */_(ER_TRANSACTION_YIELD, "Transaction has been aborted by a fiber yield") \
/*155 */_(ER_NO_SUCH_GROUP, "Replication group '%s' does not exist") \
diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 2e19d2e3c..8a008858e 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -84,12 +84,12 @@ 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;
+ /*
+ * In case of mismatch set the
+ * most strict option for
+ * is_nullable.
+ */
+ field->is_nullable = false;
}
/*
diff --git a/test/box/misc.result b/test/box/misc.result
index 4895a78a2..8934e0c87 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -398,13 +398,12 @@ t;
- 'box.error.UPDATE_ARG_TYPE : 26'
- 'box.error.CROSS_ENGINE_TRANSACTION : 81'
- 'box.error.FORMAT_MISMATCH_INDEX_PART : 27'
- - 'box.error.injection : table: <address>
- 'box.error.FUNCTION_TX_ACTIVE : 30'
+ - 'box.error.injection : table: <address>
- 'box.error.ITERATOR_TYPE : 72'
- - 'box.error.TRANSACTION_YIELD : 154'
- 'box.error.NO_SUCH_ENGINE : 57'
- 'box.error.COMMIT_IN_SUB_STMT : 122'
- - 'box.error.NULLABLE_MISMATCH : 153'
+ - 'box.error.TRANSACTION_YIELD : 154'
- 'box.error.UNSUPPORTED : 5'
- 'box.error.LAST_DROP : 15'
- 'box.error.SPACE_FIELD_IS_DUPLICATE : 149'
diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index 0cb9ed6c2..c114fb98b 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.
+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
---
@@ -1462,9 +1465,10 @@ s:insert({1, NULL})
format[1].is_nullable = true
---
...
+-- The format is allowed since in primary index parts
+-- is_nullable is still set to false.
s:format(format)
---
-- error: Field 1 is nullable in space format, but not nullable in index parts
...
format[1].is_nullable = false
---
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index a128f6000..57aa756ab 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -534,7 +534,9 @@ format[1] = { name = 'field1', type = 'unsigned', is_nullable = true }
format[2] = { name = 'field2', type = 'unsigned', is_nullable = true }
s = box.schema.space.create('test', {engine = engine, format = format})
s:create_index('primary', { parts = { 'field1' } })
-s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
+-- This is allowed, but the actual part is_nullable stays false.
+pk = s:create_index('primary', { parts = {{'field1', is_nullable = false}} })
+pk:drop()
format[1].is_nullable = false
s:format(format)
s:create_index('primary', { parts = {{'field1', is_nullable = true}} })
@@ -545,6 +547,8 @@ i.parts
-- Check that is_nullable can't be set to false on non-empty space
s:insert({1, NULL})
format[1].is_nullable = true
+-- The format is allowed since in primary index parts
+-- is_nullable is still set to false.
s:format(format)
format[1].is_nullable = false
format[2].is_nullable = false
diff --git a/test/engine/null.result b/test/engine/null.result
index 4abf85021..48d678443 100644
--- a/test/engine/null.result
+++ b/test/engine/null.result
@@ -70,49 +70,9 @@ ok
---
- false
...
--- Conflict of is_nullable in format and in parts.
-parts[1].is_nullable = false
----
-...
-sk = s:create_index('sk', { parts = parts }) -- 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,176 @@ 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, box.NULL}
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}} -- This is allowed.
+---
+...
+-- Without space format setting this fails.
+s:insert{5, box.NULL}
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+s:insert{5}
+---
+- error: Tuple field count 1 is less than required by space format or defined indexes
+ (expected at least 2)
+...
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
+---
+...
+format[2].is_nullable = true
+---
+...
+s:format(format) -- This is also allowed.
+---
+...
+-- inserts still fail due to not nullable index parts.
+s:insert{5, box.NULL}
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+s:insert{5}
+---
+- error: Tuple field count 1 is less than required by space format or defined indexes
+ (expected at least 2)
+...
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}}
+---
+...
+-- Now the field really is nullable.
+-- Success.
+s:insert{5, box.NULL}
+---
+- [5, null]
+...
+s:insert{6}
+---
+- [6]
+...
+s:insert{7, 8}
+---
+- [7, 8]
+...
+s:select{}
+---
+- - [5, null]
+ - [6]
+ - [7, 8]
+...
+-- Check that we cannot set field nullability to false when the
+-- space has tuples with NULL in this field.
+format[2].is_nullable = false
+---
+...
+s:format(format) -- Fail.
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Fail.
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+_ = s:delete{5}
+---
+...
+s:format(format) -- Still fail.
+---
+- error: Tuple field count 1 is less than required by space format or defined indexes
+ (expected at least 2)
+...
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Still fail.
+---
+- error: Tuple field count 1 is less than required by space format or defined indexes
+ (expected at least 2)
+...
+-- Now check we can set nullability to false step by step.
+_ = s:delete{6}
+---
+...
+format[2].is_nullable = false
+---
+...
+s:format(format)
+---
+...
+s:insert{5, box.NULL} -- Fail.
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+s:insert{5} -- Fail.
+---
+- error: Tuple field count 1 is less than required by space format or defined indexes
+ (expected at least 2)
+...
+format[2].is_nullable = true
+---
+...
+s:format(format)
+---
+...
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
+---
+...
+s:insert{5, box.NULL} -- Fail.
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected unsigned'
+...
+s:insert{5} -- Fail.
+---
+- error: Tuple field count 1 is less than required by space format or defined indexes
+ (expected at least 2)
+...
+format[2].is_nullable = false
+---
+...
+s:format(format)
+---
+...
+s:select{}
+---
+- - [7, 8]
+...
+s:insert{5} -- Fail.
+---
+- error: Tuple field count 1 is less than required by space format or defined indexes
+ (expected at least 2)
+...
+s:insert{9, 10} -- Success.
+---
+- [9, 10]
+...
+s:drop()
+---
+...
diff --git a/test/engine/null.test.lua b/test/engine/null.test.lua
index 7f5a7dd33..5eb068499 100644
--- a/test/engine/null.test.lua
+++ b/test/engine/null.test.lua
@@ -35,24 +35,7 @@ pk = s:create_index('pk')
ok = pcall(s.create_index, s, 'sk', { parts = parts, type = 'hash' }) -- Fail.
ok
--- Conflict of is_nullable in format and in parts.
-parts[1].is_nullable = false
-sk = s:create_index('sk', { parts = parts }) -- Fail.
-
--- 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.
-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.
-format[2].is_nullable = true
-s:format(format) -- Ok.
-- Test insert.
@@ -461,3 +444,64 @@ i2:select{}
i2:select{box.NULL, 50}
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}
+s:insert{5, box.NULL}
+
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}} -- This is allowed.
+-- Without space format setting this fails.
+s:insert{5, box.NULL}
+s:insert{5}
+
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
+format[2].is_nullable = true
+s:format(format) -- This is also allowed.
+-- inserts still fail due to not nullable index parts.
+s:insert{5, box.NULL}
+s:insert{5}
+
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=true}}}
+-- Now the field really is nullable.
+-- Success.
+s:insert{5, box.NULL}
+s:insert{6}
+s:insert{7, 8}
+s:select{}
+
+-- Check that we cannot set field nullability to false when the
+-- space has tuples with NULL in this field.
+format[2].is_nullable = false
+s:format(format) -- Fail.
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Fail.
+_ = s:delete{5}
+s:format(format) -- Still fail.
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}} -- Still fail.
+-- Now check we can set nullability to false step by step.
+_ = s:delete{6}
+
+format[2].is_nullable = false
+s:format(format)
+s:insert{5, box.NULL} -- Fail.
+s:insert{5} -- Fail.
+format[2].is_nullable = true
+s:format(format)
+s.index.secondary:alter{parts={{2, 'unsigned', is_nullable=false}}}
+s:insert{5, box.NULL} -- Fail.
+s:insert{5} -- Fail.
+format[2].is_nullable = false
+s:format(format)
+s:select{}
+s:insert{5} -- Fail.
+s:insert{9, 10} -- Success.
+
+s:drop()
--
2.15.2 (Apple Git-101.1)
More information about the Tarantool-patches
mailing list