From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <korablev@tarantool.org>
Received: from smtp58.i.mail.ru (smtp58.i.mail.ru [217.69.128.38])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 8512B445320
 for <tarantool-patches@dev.tarantool.org>;
 Wed,  5 Aug 2020 15:29:22 +0300 (MSK)
Date: Wed, 5 Aug 2020 12:29:21 +0000
From: Nikita Pettik <korablev@tarantool.org>
Message-ID: <20200805122921.GA15209@tarantool.org>
References: <20200805101251.30600-1-i.kosarev@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200805101251.30600-1-i.kosarev@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v2] tuple: drop extra restrictions
	for multikey index
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Ilya Kosarev <i.kosarev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

On 05 Aug 13:12, Ilya Kosarev wrote:
> @ChangeLog:
>  * Dropped restrictions on nullable multikey index root. All fields are
>  now nullable by default as it was before 2.2.1 (gh-5192).

I'd underline the fact that problem was fixed in another (local code
change) way.

What is more, I guess we should document this behaviour: our current
docs say nothing concerning nulls as multikey roots.
 
> diff --git a/test/engine/gh-5027-fields-nullability.test.lua b/test/engine/gh-5027-fields-nullability.test.lua
> index 960103d6c..664883d0d 100644
> --- a/test/engine/gh-5027-fields-nullability.test.lua
> +++ b/test/engine/gh-5027-fields-nullability.test.lua
> @@ -3,35 +3,18 @@ test_run = require('test_run').new()
>  s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
>  _ = s:create_index('i1', {parts={{1, 'unsigned'}}})
>  _ = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
> -s:replace{1}
> -s:replace{1, box.NULL}
> -s:replace{1, box.NULL, box.NULL}
> -s:replace{1, box.NULL, box.NULL, box.NULL}
> +s:replace{1} -- ok
> +s:replace{1, box.NULL} -- ok
> +s:replace{1, box.NULL, box.NULL} -- ok
> +s:replace{1, box.NULL, box.NULL, box.NULL} -- ok

I'd drop these -- ok comments since they only enlarge diff.

> diff --git a/test/engine/gh-5192-multikey-root-nullability.test.lua b/test/engine/gh-5192-multikey-root-nullability.test.lua
> new file mode 100644
> index 000000000..5a8305bf9
> --- /dev/null
> +++ b/test/engine/gh-5192-multikey-root-nullability.test.lua
> @@ -0,0 +1,20 @@
> +test_run = require('test_run').new()
> +
> +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
> +_ = s:format({{name='id'}, {name='data', type='array', is_nullable=true}})
> +_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +s:replace{1, box.NULL} -- ok
> +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
> +s:replace{2, box.NULL} -- ok
> +_ = s:delete(2)
> +s:truncate()

Why do you need to truncate space before drop?

> +s:drop()
> +
> +s = box.schema.space.create('gh-5027', {engine=test_run:get_cfg('engine')})
> +_ = s:format({{name='id'}, {name='data', type='array'}})
> +_ = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +s:replace{1, box.NULL} -- error
> +_ = s:create_index('i2', {parts={{field=2, path='[*].key', type='string'}}})
> +s:replace{2, box.NULL} -- error
> +s:replace{3, {}} -- ok
> +s:drop()
> \ No newline at end of file

Nit: put pls newline at the end of file to avoid this warnings.

> diff --git a/test/engine/json.test.lua b/test/engine/json.test.lua
> index 4771c3162..371bbad91 100644
> --- a/test/engine/json.test.lua
> +++ b/test/engine/json.test.lua
> @@ -220,9 +220,9 @@ s:insert{4, {}} -- error
>  s:insert{5, {{b = 1}}} -- error
>  s:insert{6, {{a = 1}}} -- ok
>  s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}}
> -s:insert{7, box.NULL} -- error
> -s:insert{8, {box.NULL}} -- error
> --- Skipping nullable fields is okay though.
> +s:insert{7, box.NULL} -- ok
> +s:insert{8, {box.NULL}} -- ok
> +-- Skipping nullable fields is also okay.

Is this change really required? :)

>  s:insert{9} -- ok
>  s:insert{10, {}} -- ok
>  s:insert{11, {{b = 1}}} -- ok