[Tarantool-patches] [PATCH v2 2/4] tuple: pass tuple format to xrow_update_*_store()

[Vladislav Shpilevoy]

Thanks for the review!

On 10/02/2020 17:51, Nikita Pettik wrote:
> On 04 Feb 23:53, Vladislav Shpilevoy wrote:
>> diff --git a/src/box/xrow_update_array.c b/src/box/xrow_update_array.c
>> index b97c23d77..717466be7 100644
>> --- a/src/box/xrow_update_array.c
>> +++ b/src/box/xrow_update_array.c
>> @@ -31,6 +31,7 @@
>>  #include "xrow_update_field.h"
>>  #include "msgpuck.h"
>>  #include "fiber.h"
>> +#include "tuple_format.h"
>>  
>>  /**
>>   * Make sure @a op contains a valid field number to where the
>> @@ -259,26 +260,49 @@ xrow_update_array_sizeof(struct xrow_update_field *field)
>>  }
>>  
>>  uint32_t
>> -xrow_update_array_store(struct xrow_update_field *field, char *out,
>> -			char *out_end)
>> +xrow_update_array_store(struct xrow_update_field *field,
>> +			struct json_tree *format_tree,
>> +			struct json_token *this_node, char *out, char *out_end)
>>  {
>>  	assert(field->type == XUPDATE_ARRAY);
>>  	char *out_begin = out;
>>  	out = mp_encode_array(out, xrow_update_rope_size(field->array.rope));
>> -	uint32_t total_field_count = 0;
>>  	struct xrow_update_rope_iter it;
>>  	xrow_update_rope_iter_create(&it, field->array.rope);
>>  	struct xrow_update_rope_node *node = xrow_update_rope_iter_start(&it);
>> -	for (; node != NULL; node = xrow_update_rope_iter_next(&it)) {
>> -		struct xrow_update_array_item *item =
>> -			xrow_update_rope_leaf_data(node);
>> -		uint32_t field_count = xrow_update_rope_leaf_size(node);
>> -		out += xrow_update_field_store(&item->field, out, out_end);
>> -		assert(item->tail_size == 0 || field_count > 1);
>> -		memcpy(out, item->field.data + item->field.size,
>> -		       item->tail_size);
>> -		out += item->tail_size;
>> -		total_field_count += field_count;
>> +	uint32_t total_field_count = 0;
>> +	if (this_node == NULL) {
>> +		for (; node != NULL; node = xrow_update_rope_iter_next(&it)) {
>> +			struct xrow_update_array_item *item =
>> +				xrow_update_rope_leaf_data(node);
>> +			uint32_t field_count = xrow_update_rope_leaf_size(node);
>> +			out += xrow_update_field_store(&item->field, NULL, NULL,
>> +						       out, out_end);
>> +			assert(item->tail_size == 0 || field_count > 1);
>> +			memcpy(out, item->field.data + item->field.size,
>> +			       item->tail_size);
>> +			out += item->tail_size;
>> +			total_field_count += field_count;
>> +		}
>> +	} else {
>> +		struct json_token token;
>> +		token.type = JSON_TOKEN_NUM;
>> +		token.num = 0;
>> +		struct json_token *next_node;
>> +		for (; node != NULL; node = xrow_update_rope_iter_next(&it)) {
>> +			struct xrow_update_array_item *item =
>> +				xrow_update_rope_leaf_data(node);
>> +			next_node = json_tree_lookup(format_tree, this_node, &token);
>> +			uint32_t field_count = xrow_update_rope_leaf_size(node);
>> +			out += xrow_update_field_store(&item->field, format_tree,
>> +						       next_node, out, out_end);
>> +			assert(item->tail_size == 0 || field_count > 1);
>> +			memcpy(out, item->field.data + item->field.size,
>> +			       item->tail_size);
>> +			out += item->tail_size;
>> +			token.num += field_count;
>> +			total_field_count += field_count;
>> +		}
> 
> IMHO this code duplication looks a bit rough. What about next refactoring:

I did it intentionally to avoid comparison of 'this_node' with NULL on each
iteration. But ok, applied your diff.

I also found a bug in this patch - I used tuple index base 0 for JSON
paths lookup in xrow_update_bar/route.c. The test in the last patch was
broken because of this, but I missed that, because it was incorrect as well.
Fixed the bug here, and the test there. Diff below.

================================================================================
diff --git a/src/box/xrow_update_bar.c b/src/box/xrow_update_bar.c
index b4a98978a..f104a08da 100644
--- a/src/box/xrow_update_bar.c
+++ b/src/box/xrow_update_bar.c
@@ -436,7 +436,7 @@ xrow_update_bar_store(struct xrow_update_field *field,
 		if (this_node != NULL) {
 			this_node = json_tree_lookup_path(
 				format_tree, this_node, field->bar.path,
-				field->bar.path_len, 0);
+				field->bar.path_len, TUPLE_INDEX_BASE);
 		}
diff --git a/src/box/xrow_update_route.c b/src/box/xrow_update_route.c
index 122f0329e..d2b8979b8 100644
--- a/src/box/xrow_update_route.c
+++ b/src/box/xrow_update_route.c
@@ -384,7 +384,7 @@ xrow_update_route_store(struct xrow_update_field *field,
 	if (this_node != NULL) {
 		this_node = json_tree_lookup_path(
 			format_tree, this_node, field->route.path,
-			field->route.path_len, 0);
+			field->route.path_len, TUPLE_INDEX_BASE);
 	}
================================================================================

Additionally, I realized, that I don't need the json tree in upsert squash.
Indeed, it merges operations, not fields. Providing type of a result field
won't help here, because anyway it will be provided, when the upsert is
merged with some real tuple. Diff below.

================================================================================
diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c
index ac8c9d303..3cdcc9ee7 100644
--- a/src/box/xrow_update.c
+++ b/src/box/xrow_update.c
@@ -467,10 +467,6 @@ xrow_upsert_squash(const char *expr1, const char *expr1_end,
 
 	uint32_t op_count[2] = {update[0].op_count, update[1].op_count};
 	uint32_t op_no[2] = {0, 0};
-	struct json_tree *format_tree = &format->fields;
-	struct json_token *root = &format_tree->root;
-	struct json_token token;
-	token.type = JSON_TOKEN_NUM;
 	while (op_no[0] < op_count[0] || op_no[1] < op_count[1]) {
 		res_count++;
 		struct xrow_update_op *op[2] = {update[0].ops + op_no[0],
@@ -525,13 +521,10 @@ xrow_upsert_squash(const char *expr1, const char *expr1_end,
 		res_ops = mp_encode_array(res_ops, 3);
 		res_ops = mp_encode_str(res_ops,
 					(const char *)&op[0]->opcode, 1);
-		token.num = op[0]->field_no;
-		res_ops = mp_encode_uint(res_ops, token.num +
-					 update[0].index_base);
-		struct json_token *this_node =
-			json_tree_lookup(format_tree, root, &token);
-		xrow_update_op_store_arith(&res, format_tree, this_node, NULL,
-					   res_ops);
+		res_ops = mp_encode_uint(res_ops,
+					 op[0]->field_no +
+						 update[0].index_base);
+		xrow_update_op_store_arith(&res, NULL, NULL, NULL, res_ops);
 		res_ops += xrow_update_arg_arith_sizeof(&res.arg.arith);
 		mp_next(&expr[0]);
 		mp_next(&expr[1]);
================================================================================