From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 D63C546970E for ; Tue, 11 Feb 2020 00:18:47 +0300 (MSK) References: <79fa040657f8826cb03bcd4cfd1d572e8cda69ce.1580856721.git.v.shpilevoy@tarantool.org> <20200210165142.GA99436@tarantool.org> From: Vladislav Shpilevoy Message-ID: <8e0524c3-6c6f-912f-4b39-ce95a8f58849@tarantool.org> Date: Mon, 10 Feb 2020 22:18:45 +0100 MIME-Version: 1.0 In-Reply-To: <20200210165142.GA99436@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v2 2/4] tuple: pass tuple format to xrow_update_*_store() List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org 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]); ================================================================================