From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 7A58022C40 for ; Wed, 8 Aug 2018 08:50:29 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id FjfpROmiBaT0 for ; Wed, 8 Aug 2018 08:50:29 -0400 (EDT) Received: from smtp47.i.mail.ru (smtp47.i.mail.ru [94.100.177.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 367FA22BB5 for ; Wed, 8 Aug 2018 08:50:29 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Allow nullability mismatch in space indices and format. References: <20180807144829.34260-1-sergepetrenko@tarantool.org> From: Vladislav Shpilevoy Message-ID: <8702b347-f250-8f9f-3571-17013e63d8c4@tarantool.org> Date: Wed, 8 Aug 2018 15:50:26 +0300 MIME-Version: 1.0 In-Reply-To: <20180807144829.34260-1-sergepetrenko@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Serge Petrenko , tarantool-patches@freelists.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.