Tarantool development patches archive
 help / color / mirror / Atom feed
From: Aleksandr Lyapunov <alyapunov@tarantool.org>
To: Ilya Kosarev <i.kosarev@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map
Date: Tue, 23 Jun 2020 13:56:49 +0300	[thread overview]
Message-ID: <fa887161-2049-a64c-8f42-85f6182afa96@tarantool.org> (raw)
In-Reply-To: <20200618175720.27089-1-i.kosarev@tarantool.org>

Hello, thanks for the patch!
I don't like it nevertheless.
Btw I don't like Vladimir's patch about required map/array either.
It seems that there are to many ways to declare field as nullable,
at least by setting nullable_action and removing corresponding bit
from required_fileds mask.
There must one more way since tarantool from master allows
to insert tuples like that:
s:create_index('pk', {parts={{1, uint}}})
s:create_index('test', {parts={{4, uint}}})
s:replace{1, box.NULL, nil, 1}

That means that the case above 2nd and 3rd fields are nullable
by default.

It all looks like a mess, I fear that you have to dig more and create
the one and only way to set fields as required or not.

Please see my comment below.

On 6/18/20 8:57 PM, Ilya Kosarev wrote:
> Since e1d3fe8ab8eed65394ad17409401a93b6fcdc435 (tuple format: don't
> allow null where array/map is expected) tuple fields are nullable by
> default. It seems strange at least in case we have implicit fields in
> front of explicit nullable field. Now fields are nullable by default
> except arrays & maps due to reasons specified in mentioned commit.
>
> Closes #5027
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5027-fix-tuple-fields-nullability
> Issue: https://github.com/tarantool/tarantool/issues/5027
>
> @ChangeLog:
>   * Fix confusing implicit requirements for tuple fields.
>
>   src/box/tuple_format.c      |  6 +++++-
>   test/engine/insert.result   | 33 +++++++++++++++++++++++++++++++++
>   test/engine/insert.test.lua | 12 ++++++++++++
>   3 files changed, 50 insertions(+), 1 deletion(-)
>
> diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
> index 68ec2a7499..4b8f32149e 100644
> --- a/src/box/tuple_format.c
> +++ b/src/box/tuple_format.c
> @@ -152,7 +152,7 @@ tuple_field_new(void)
>   	field->type = FIELD_TYPE_ANY;
>   	field->offset_slot = TUPLE_OFFSET_SLOT_NIL;
>   	field->coll_id = COLL_NONE;
> -	field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
> +	field->nullable_action = ON_CONFLICT_ACTION_NONE;
>   	field->multikey_required_fields = NULL;
>   	return field;
>   }
> @@ -528,6 +528,10 @@ tuple_format_create(struct tuple_format *format, struct key_def * const *keys,
>   				multikey_required_fields;
>   			required_fields = multikey_required_fields;
>   		}
> +		if ((field->type == FIELD_TYPE_ARRAY ||
> +		    field->type == FIELD_TYPE_MAP) &&
> +		    tuple_field_is_nullable(field))
> +			field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
That doesn't look like a proper place to edit field.
If array and map fields are not allowed to be nullable then two lines
above the filed was in inconsistent state - with array/map type and 
nullable.
That's bad. We should set type and nullable_action at least in adjacent 
lines.
>   		/*
>   		 * Mark all leaf non-nullable fields as required
>   		 * by setting the corresponding bit in the bitmap
> diff --git a/test/engine/insert.result b/test/engine/insert.result
> index 13ffe8ceb1..37a4e8cc68 100644
> --- a/test/engine/insert.result
> +++ b/test/engine/insert.result
> @@ -988,3 +988,36 @@ s:select()
>   s:drop()
>   ---
>   ...
> +-- gh-5027: Do not require extra fields
> +s = box.schema.space.create('test')
> +---
> +...
> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +---
> +...
> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
> +---
> +...
> +s:insert{1, 2, 3}
> +---
> +- [1, 2, 3]
> +...
> +s:drop()
> +---
> +...
> +s = box.schema.space.create('test')
> +---
> +...
> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +---
> +...
> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}})
> +---
> +...
> +s:insert{1, 2, 3}
> +---
> +- error: Tuple field 5 required by space format is missing
> +...
> +s:drop()
> +---
> +...
> diff --git a/test/engine/insert.test.lua b/test/engine/insert.test.lua
> index 2ffc8cd0a0..112ca62c53 100644
> --- a/test/engine/insert.test.lua
> +++ b/test/engine/insert.test.lua
> @@ -210,3 +210,15 @@ ddd:delete(ffi.cast('double', 123))
>   s:select()
>   
>   s:drop()
> +
> +-- gh-5027: Do not require extra fields
> +s = box.schema.space.create('test')
> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=true}}})
> +s:insert{1, 2, 3}
> +s:drop()
> +s = box.schema.space.create('test')
> +i1 = s:create_index('i1', {parts={{1, 'unsigned'}}})
> +i2 = s:create_index('i2', {parts={{5, 'unsigned', is_nullable=false}}})
> +s:insert{1, 2, 3}
> +s:drop()
> \ No newline at end of file

  reply	other threads:[~2020-06-23 10:56 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-18 17:57 Ilya Kosarev
2020-06-23 10:56 ` Aleksandr Lyapunov [this message]
2020-06-29 17:42   ` Ilya Kosarev
2020-07-03  9:28 ` Nikita Pettik
2020-07-12 13:43   ` Ilya Kosarev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fa887161-2049-a64c-8f42-85f6182afa96@tarantool.org \
    --to=alyapunov@tarantool.org \
    --cc=i.kosarev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] tuple: make fields nullable by default except array/map' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox