From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <korablev@tarantool.org>
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 dev.tarantool.org (Postfix) with ESMTPS id EB6C2445320
 for <tarantool-patches@dev.tarantool.org>;
 Mon, 13 Jul 2020 02:11:54 +0300 (MSK)
Date: Sun, 12 Jul 2020 23:11:53 +0000
From: Nikita Pettik <korablev@tarantool.org>
Message-ID: <20200712231153.GC13229@tarantool.org>
References: <e32f85595b4fce26790dccf5968ed1405e2eda86.1593883084.git.v.shpilevoy@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <e32f85595b4fce26790dccf5968ed1405e2eda86.1593883084.git.v.shpilevoy@tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH 1/1] tuple: turn invalid bar update
	into nop
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: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org

On 04 Jul 19:20, Vladislav Shpilevoy wrote:

LGTM

> While working on this I found an inconsistency in which errors do
> we ignore. https://github.com/tarantool/tarantool/issues/5137

Great!
 
> diff --git a/src/box/xrow_update_bar.c b/src/box/xrow_update_bar.c
> index 0033f0044..1d20d6c74 100644
> --- a/src/box/xrow_update_bar.c
> +++ b/src/box/xrow_update_bar.c
> @@ -31,6 +31,22 @@
>  #include "xrow_update_field.h"
>  #include "tuple.h"
>  
> +/**
> + * 'Commit' bar creation only when it is fully initialized and
> + * valid. Because if all this is happening inside an upsert()
> + * operation, it won't stop the whole xrow upsert. This field will
> + * still be saved in the result tuple. But in case of an error
> + * this operation should be skipped. So this is kept 'nop' when
> + * error happens.
> + */
> +static inline int
> +xrow_update_bar_commit(struct xrow_update_field *field)

Mb _done/_finish are better names? Up to you. 

> +{
> +	assert(field->type == XUPDATE_NOP);
> +	field->type = XUPDATE_BAR;
> +	return 0;
> +}
> +
>  /**
>   * Locate a field to update by @a op's JSON path and initialize
>   * @a field as a bar update.
> diff --git a/test/box/update.test.lua b/test/box/update.test.lua
> index 1538cae9c..27cf55467 100644
> --- a/test/box/update.test.lua
> +++ b/test/box/update.test.lua
> @@ -343,20 +343,52 @@ t2:update({{'!', 'g[6][1]', 50}})
>  t4_array:update({{'!', '[4][1]', 100}})
>  t4_map:update({{'!', '[4].a', 100}})
>  -- Test errors.
> -t:update({{'!', 'a', 100}}) -- No such field.
> -t:update({{'!', 'f.a', 300}}) -- Key already exists.
> -t:update({{'!', 'f.c.f[0]', 3.5}}) -- No such index, too small.
> -t:update({{'!', 'f.c.f[100]', 100}}) -- No such index, too big.
> -t:update({{'!', 'g[4][100]', 700}}) -- Insert index into map.
> -t:update({{'!', 'g[1][1]', 300}})
> -t:update({{'!', 'f.g.a', 700}}) -- Insert key into array.
> -t:update({{'!', 'f.g[1].a', 700}})
> -t:update({{'!', 'f[*].k', 20}}) -- 'Any' is not considered valid JSON.

New tests are cool, but mb it is worth reducing diff by placing
new tests right after old ones? Don't insist tho.