From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Wed, 23 Jan 2019 17:15:10 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [tarantool-patches] [PATCH v8 6/6] box: introduce has_json_paths
 flag in templates
Message-ID: <20190123141510.tlsket6ah5dof4yx@esperanza>
References: <cover.1547645795.git.kshcherbatov@tarantool.org>
 <6e7f31aa-0793-8cab-076a-c9967daff33c@tarantool.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6e7f31aa-0793-8cab-076a-c9967daff33c@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

On Wed, Jan 16, 2019 at 06:35:56PM +0300, Kirill Shcherbatov wrote:
> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index 7ab6e3bf6..dff3aebbd 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -458,11 +458,12 @@ tuple_common_key_parts(const struct tuple *tuple_a, const struct tuple *tuple_b,
>  	return i;
>  }
>  
> -template<bool is_nullable, bool has_optional_parts>
> +template<bool is_nullable, bool has_optional_parts, bool has_json_path>
>  static inline int
>  tuple_compare_slowpath(const struct tuple *tuple_a, const struct tuple *tuple_b,
>  		       struct key_def *key_def)
>  {
> +	assert(has_json_path == key_def->has_json_paths);

Sometimes you use 'has_json_paths', sometimes 'has_json_path'
(without 's'). Please be consistent.

>  	assert(!has_optional_parts || is_nullable);
>  	assert(is_nullable == key_def->is_nullable);
>  	assert(has_optional_parts == key_def->has_optional_parts);
> @@ -508,10 +509,19 @@ tuple_compare_slowpath(const struct tuple *tuple_a, const struct tuple *tuple_b,
>  		end = part + key_def->part_count;
>  
>  	for (; part < end; part++) {
> -		field_a = tuple_field_by_part_raw(format_a, tuple_a_raw,
> -						  field_map_a, part);
> -		field_b = tuple_field_by_part_raw(format_b, tuple_b_raw,
> -						  field_map_b, part);
> +		if (!has_json_path) {
> +			field_a = tuple_field_raw(format_a, tuple_a_raw,
> +						  field_map_a,
> +						  part->fieldno);

No need to carry part->fieldno to the next line - it fits.

> +			field_b = tuple_field_raw(format_b, tuple_b_raw,
> +						  field_map_b,
> +						  part->fieldno);

Ditto.

> +		} else {
> +			field_a = tuple_field_by_part_raw(format_a, tuple_a_raw,
> +							  field_map_a, part);
> +			field_b = tuple_field_by_part_raw(format_b, tuple_b_raw,
> +							  field_map_b, part);
> +		}
>  		assert(has_optional_parts ||
>  		       (field_a != NULL && field_b != NULL));
>  		if (! is_nullable) {
> diff --git a/src/box/tuple_extract_key.cc b/src/box/tuple_extract_key.cc
> index c10ee8ce4..78462908d 100644
> --- a/src/box/tuple_extract_key.cc
> +++ b/src/box/tuple_extract_key.cc
> @@ -133,7 +145,8 @@ tuple_extract_key_slowpath(const struct tuple *tuple,
>  			 * minimize tuple_field_raw() calls.
>  			 */
>  			for (; i < part_count - 1; i++) {
> -				if (!key_def_parts_are_sequential(key_def, i)) {
> +				if (!key_def_parts_are_sequential
> +				        <has_json_paths>(key_def, i)) {

Bad alignment - looks like <has_json_paths>(key_def, i) isn't a part of
'if' condition. Please align by the parenthesis. Here and everywhere
else in the patch.

> diff --git a/src/box/tuple_hash.cc b/src/box/tuple_hash.cc
> index 3486ce11c..8ede29045 100644
> --- a/src/box/tuple_hash.cc
> +++ b/src/box/tuple_hash.cc
> @@ -347,9 +359,18 @@ tuple_hash_slowpath(const struct tuple *tuple, struct key_def *key_def)
>  		 * need of tuple_field
>  		 */
>  		if (prev_fieldno + 1 != key_def->parts[part_id].fieldno) {
> -			struct key_part *part = &key_def->parts[part_id];
> -			field = tuple_field_by_part_raw(format, tuple_raw,
> -							field_map, part);
> +			if (!has_json_paths) {
> +				field = tuple_field(tuple,
> +						    key_def->parts[part_id].
> +						    fieldno);

Please don't carry .fieldno over to the next line - it looks confusing.
If you didn't remove local variable 'part', you would avoid that.

> +			} else {
> +				struct key_part *part =
> +					&key_def->parts[part_id];
> +				field = tuple_field_by_part_raw(format,
> +								tuple_raw,
> +								field_map,
> +								part);
> +			}
>  		}
>  		if (has_optional_parts && (field == NULL || field >= end)) {
>  			total_size += tuple_hash_null(&h, &carry);