[Tarantool-patches] [PATCH v2 0/4] Don't truncate float in update

[Tarantool-patches] [PATCH v2 0/4] Don't truncate float in update

[Vladislav Shpilevoy]

The patchset fixes float value truncation in
tuple/space:update/upsert. First commit fixes the actual issue,
but introduces another bug that a float value can be stored into a
field having double type in its format.

The rest 3 commits fix this new bug.

However, there is an alternative how these 3 could be done. My
solution is that I calculate the result types in
xrow_update_*_store() functions. Because of that I don't know the
types during xrow_update_*_sizeof(), and it needs to return the
maximal possible size instead of exact size. The alternative is to
calculate types during sizeof. It would allow to return the exact
size. But it also would complicate the code. I can implement this
option if it looks better.

Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4701-update-float-truncate
Issue: https://github.com/tarantool/tarantool/issues/4701

Changes in V2:
- Taken into account, that a field may have double type in its
  tuple format, and can't store floats at all.

Vladislav Shpilevoy (4):
  tuple: don't truncate float in :update()
  tuple: pass tuple format to xrow_update_*_store()
  tuple: allow xrow_update sizeof reserve more memory
  tuple: use field type in update of a float/double

 src/box/lua/tuple.c         |   2 +-
 src/box/memtx_space.c       |   7 ++-
 src/box/session_settings.c  |   2 +-
 src/box/space.c             |   6 +--
 src/box/tuple.c             |   4 +-
 src/box/vinyl.c             |   9 ++--
 src/box/vy_upsert.c         |   4 +-
 src/box/xrow_update.c       |  43 +++++++++------
 src/box/xrow_update.h       |  10 ++--
 src/box/xrow_update_array.c |  50 ++++++++++++-----
 src/box/xrow_update_bar.c   |  13 +++--
 src/box/xrow_update_field.c | 102 ++++++++++++++++++++++++-----------
 src/box/xrow_update_field.h |  22 +++++---
 src/box/xrow_update_map.c   |  34 ++++++++++--
 src/box/xrow_update_route.c |  13 +++--
 test/box/update.result      | 103 ++++++++++++++++++++++++++++++++++++
 test/box/update.test.lua    |  62 ++++++++++++++++++++++
 test/unit/column_mask.c     |   4 +-
 18 files changed, 388 insertions(+), 102 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 1/4] tuple: don't truncate float in :update()

[Vladislav Shpilevoy]

Before the patch there were the rules:
* float +/- double = double
* double +/- double = double
* float +/- float = float

The rules were applied regardless of values. That led to a problem
when float + float exceeding maximal float value could fit into
double, but was stored as an infinity.

The patch makes so that if a floating point arithmetic operation
result fits into float, it is stored as float. Otherwise as
double. Regardless of initial types.

This alongside saves some memory for cases when doubles can be
stored as floats, and therefore takes 4 less bytes. Although
these cases are rare, because any not integer value stored in a
double may have a long garbage tail in its fraction.

Closes #4701
---
 src/box/xrow_update_field.c | 10 ++++--
 test/box/update.result      | 65 +++++++++++++++++++++++++++++++++++++
 test/box/update.test.lua    | 45 +++++++++++++++++++++++++
 3 files changed, 117 insertions(+), 3 deletions(-)

diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index 7c0f5fb5e..6ac29de5d 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -32,6 +32,8 @@
 #include "tuple_format.h"
 #include "mp_extension_types.h"
 
+#include <float.h>
+
 /* {{{ Error helpers. */
 
 /** Take a string identifier of a field being updated by @a op. */
@@ -400,13 +402,15 @@ xrow_update_arith_make(struct xrow_update_op *op,
 			unreachable();
 			break;
 		}
-		if (lowest_type == XUPDATE_TYPE_DOUBLE) {
+		float fc = (float) c;
+		if ((lowest_type == XUPDATE_TYPE_DOUBLE && c != (double) fc) ||
+		    (lowest_type == XUPDATE_TYPE_FLOAT &&
+		     (c >= FLT_MAX || c <= -FLT_MAX))) {
 			ret->type = XUPDATE_TYPE_DOUBLE;
 			ret->dbl = c;
 		} else {
-			assert(lowest_type == XUPDATE_TYPE_FLOAT);
 			ret->type = XUPDATE_TYPE_FLOAT;
-			ret->flt = (float)c;
+			ret->flt = fc;
 		}
 	} else {
 		decimal_t a, b, c;
diff --git a/test/box/update.result b/test/box/update.result
index 28ba47831..dfbd8714f 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -1647,3 +1647,68 @@ t:update({{'=', '[2].d.g', 6000}, {'#', '[2].d.old.old', 1}})
 s:drop()
 ---
 ...
+--
+-- gh-4701: arith operations should not truncate result when
+-- float + float fits double.
+--
+msgpackffi = require('msgpackffi')
+---
+...
+mp_array_1 = 0x91
+---
+...
+mp_double = 0xcb
+---
+...
+mp_float = 0xca
+---
+...
+flt_max = 3.402823466e+38
+---
+...
+uint_max = 18446744073709551615ULL
+---
+...
+-- Double + double is float if result fits.                                     \
+-- Double + double is double if result does not fit float.                      \
+-- Double + float is float if result fits.                                      \
+-- Double + float is double if result does not fit float.                       \
+-- Float + float is float when no overflow.                                     \
+-- Float + float is double when overflow.                                       \
+-- Float + int is float when fits the float range.                              \
+-- Precision matters too. Double is used when need to avoid                     \
+-- precision loss.                                                              \
+tests = {                                                                       \
+    {{'double', 1}, {'double', 1}, mp_float},                                   \
+    {{'double', flt_max}, {'double', flt_max}, mp_double},                      \
+    {{'double', 1}, {'float', 1}, mp_float},                                    \
+    {{'double', flt_max}, {'float', flt_max}, mp_double},                       \
+    {{'float', 1}, {'float', 1}, mp_float},                                     \
+    {{'float', flt_max}, {'float', flt_max}, mp_double},                        \
+    {{'float', -flt_max}, {'float', -flt_max}, mp_double},                      \
+    {{'float', 1}, {'int', 1}, mp_float},                                       \
+    {{'float', flt_max}, {'uint64_t', uint_max}, mp_double},                    \
+    {{'float', 1.0001}, {'double', 1.0000000000001}, mp_double},                \
+}
+---
+...
+err = nil
+---
+...
+for i, test in pairs(tests) do                                                  \
+    local val1 = ffi.cast(test[1][1], test[1][2])                               \
+    local val2 = ffi.cast(test[2][1], test[2][2])                               \
+    local t = box.tuple.new({val1})                                             \
+    t = t:update({{'+', 1, val2}})                                              \
+    local m = msgpackffi.encode(t)                                              \
+    if m:byte(1) ~= mp_array_1 or m:byte(2) ~= test[3] then                     \
+        err = {i, test, t, m:byte(1), m:byte(2)}                                   \
+        break                                                                   \
+    end                                                                         \
+end
+---
+...
+err
+---
+- null
+...
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index 314ebef05..74f9c62e2 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -597,3 +597,48 @@ t:update({{'=', '[2].d.g', 6000}, {'=', '[2].d.new.new', -1}})
 t:update({{'=', '[2].d.g', 6000}, {'#', '[2].d.old.old', 1}})
 
 s:drop()
+
+--
+-- gh-4701: arith operations should not truncate result when
+-- float + float fits double.
+--
+msgpackffi = require('msgpackffi')
+mp_array_1 = 0x91
+mp_double = 0xcb
+mp_float = 0xca
+flt_max = 3.402823466e+38
+uint_max = 18446744073709551615ULL
+tests = {                                                                       \
+-- Double + double is float if result fits.                                     \
+    {{'double', 1}, {'double', 1}, mp_float},                                   \
+-- Double + double is double if result does not fit float.                      \
+    {{'double', flt_max}, {'double', flt_max}, mp_double},                      \
+-- Double + float is float if result fits.                                      \
+    {{'double', 1}, {'float', 1}, mp_float},                                    \
+-- Double + float is double if result does not fit float.                       \
+    {{'double', flt_max}, {'float', flt_max}, mp_double},                       \
+-- Float + float is float when no overflow.                                     \
+    {{'float', 1}, {'float', 1}, mp_float},                                     \
+-- Float + float is double when overflow.                                       \
+    {{'float', flt_max}, {'float', flt_max}, mp_double},                        \
+    {{'float', -flt_max}, {'float', -flt_max}, mp_double},                      \
+-- Float + int is float when fits the float range.                              \
+    {{'float', 1}, {'int', 1}, mp_float},                                       \
+    {{'float', flt_max}, {'uint64_t', uint_max}, mp_double},                    \
+-- Precision matters too. Double is used when need to avoid                     \
+-- precision loss.                                                              \
+    {{'float', 1.0001}, {'double', 1.0000000000001}, mp_double},                \
+}
+err = nil
+for i, test in pairs(tests) do                                                  \
+    local val1 = ffi.cast(test[1][1], test[1][2])                               \
+    local val2 = ffi.cast(test[2][1], test[2][2])                               \
+    local t = box.tuple.new({val1})                                             \
+    t = t:update({{'+', 1, val2}})                                              \
+    local m = msgpackffi.encode(t)                                              \
+    if m:byte(1) ~= mp_array_1 or m:byte(2) ~= test[3] then                     \
+        err = {i, test, t, m:byte(1), m:byte(2)}                                   \
+        break                                                                   \
+    end                                                                         \
+end
+err
-- 
2.21.1 (Apple Git-122.3)

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

[Vladislav Shpilevoy]

Tuple format now is passed to xrow_update routines. It is going to
be used for two purposes:

- Find field types of the result tuple fields. It will be used to
  decide whether a floating point value should be saved with
  single or double precision;

- In future use the format and tuple offset map to find target
  fields for O(1), without decoding anything. May be especially
  useful for JSON updates of indexed fields.

For the types the format is passed to *_store() functions. Types
can't be calculated earlier, because '!' and '#' operations change
field order. Even if a type would be calculated during operations
appliance for field N, an operation '!' or '#' on field < N would
make this calculation useless.

Follow-up #4701
---
 src/box/lua/tuple.c         |  2 +-
 src/box/memtx_space.c       |  7 +++---
 src/box/session_settings.c  |  2 +-
 src/box/space.c             |  6 ++---
 src/box/tuple.c             |  4 +--
 src/box/vinyl.c             |  9 +++----
 src/box/vy_upsert.c         |  4 +--
 src/box/xrow_update.c       | 41 +++++++++++++++++++-----------
 src/box/xrow_update.h       | 10 ++++----
 src/box/xrow_update_array.c | 50 +++++++++++++++++++++++++++----------
 src/box/xrow_update_bar.c   | 12 +++++++--
 src/box/xrow_update_field.c | 46 ++++++++++++++++++++++++++--------
 src/box/xrow_update_field.h | 18 +++++++++----
 src/box/xrow_update_map.c   | 34 +++++++++++++++++++++----
 src/box/xrow_update_route.c | 13 +++++++---
 test/unit/column_mask.c     |  4 +--
 16 files changed, 183 insertions(+), 79 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 1f9d6e662..1cbdacd0b 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -454,7 +454,7 @@ lbox_tuple_transform(struct lua_State *L)
 	 */
 	const char *new_data =
 		xrow_update_execute(buf->buf, buf->buf + ibuf_used(buf),
-				    old_data, old_data + bsize, format->dict,
+				    old_data, old_data + bsize, format,
 				    &new_size, 1, NULL);
 	if (new_data != NULL)
 		new_tuple = tuple_new(box_tuple_format_default(),
diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
index 6ef84e045..7c28b7d7b 100644
--- a/src/box/memtx_space.c
+++ b/src/box/memtx_space.c
@@ -392,7 +392,7 @@ memtx_space_execute_update(struct space *space, struct txn *txn,
 	const char *old_data = tuple_data_range(old_tuple, &bsize);
 	const char *new_data =
 		xrow_update_execute(request->tuple, request->tuple_end,
-				    old_data, old_data + bsize, format->dict,
+				    old_data, old_data + bsize, format,
 				    &new_size, request->index_base, NULL);
 	if (new_data == NULL)
 		return -1;
@@ -462,8 +462,7 @@ memtx_space_execute_upsert(struct space *space, struct txn *txn,
 		 * @sa https://github.com/tarantool/tarantool/issues/1156
 		 */
 		if (xrow_update_check_ops(request->ops, request->ops_end,
-					  format->dict,
-					  request->index_base) != 0) {
+					  format, request->index_base) != 0) {
 			return -1;
 		}
 		stmt->new_tuple = memtx_tuple_new(format, request->tuple,
@@ -484,7 +483,7 @@ memtx_space_execute_upsert(struct space *space, struct txn *txn,
 		const char *new_data =
 			xrow_upsert_execute(request->ops, request->ops_end,
 					    old_data, old_data + bsize,
-					    format->dict, &new_size,
+					    format, &new_size,
 					    request->index_base, false,
 					    &column_mask);
 		if (new_data == NULL)
diff --git a/src/box/session_settings.c b/src/box/session_settings.c
index a969d3d10..37d2a3e85 100644
--- a/src/box/session_settings.c
+++ b/src/box/session_settings.c
@@ -382,7 +382,7 @@ session_settings_space_execute_update(struct space *space, struct txn *txn,
 found:
 	module->get(sid, &old_data, &old_data_end);
 	new_data = xrow_update_execute(request->tuple, request->tuple_end,
-				       old_data, old_data_end, format->dict,
+				       old_data, old_data_end, format,
 				       &new_size, request->index_base,
 				       &column_mask);
 	if (new_data == NULL)
diff --git a/src/box/space.c b/src/box/space.c
index 1c19d099b..eecbde7fa 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -409,7 +409,7 @@ space_before_replace(struct space *space, struct txn *txn,
 		new_data = xrow_update_execute(request->tuple,
 					       request->tuple_end, old_data,
 					       old_data_end,
-					       space->format->dict, &new_size,
+					       space->format, &new_size,
 					       request->index_base, NULL);
 		if (new_data == NULL)
 			return -1;
@@ -432,7 +432,7 @@ space_before_replace(struct space *space, struct txn *txn,
 			new_data_end = request->tuple_end;
 			if (xrow_update_check_ops(request->ops,
 						  request->ops_end,
-						  space->format->dict,
+						  space->format,
 						  request->index_base) != 0)
 				return -1;
 			break;
@@ -441,7 +441,7 @@ space_before_replace(struct space *space, struct txn *txn,
 		old_data_end = old_data + old_size;
 		new_data = xrow_upsert_execute(request->ops, request->ops_end,
 					       old_data, old_data_end,
-					       space->format->dict, &new_size,
+					       space->format, &new_size,
 					       request->index_base, false,
 					       NULL);
 		new_data_end = new_data + new_size;
diff --git a/src/box/tuple.c b/src/box/tuple.c
index 4d676b090..1f52a8cff 100644
--- a/src/box/tuple.c
+++ b/src/box/tuple.c
@@ -692,7 +692,7 @@ box_tuple_update(box_tuple_t *tuple, const char *expr, const char *expr_end)
 	struct tuple_format *format = tuple_format(tuple);
 	const char *new_data =
 		xrow_update_execute(expr, expr_end, old_data, old_data + bsize,
-				    format->dict, &new_size, 1, NULL);
+				    format, &new_size, 1, NULL);
 	if (new_data == NULL) {
 		region_truncate(region, used);
 		return NULL;
@@ -714,7 +714,7 @@ box_tuple_upsert(box_tuple_t *tuple, const char *expr, const char *expr_end)
 	struct tuple_format *format = tuple_format(tuple);
 	const char *new_data =
 		xrow_upsert_execute(expr, expr_end, old_data, old_data + bsize,
-				    format->dict, &new_size, 1, false, NULL);
+				    format, &new_size, 1, false, NULL);
 	if (new_data == NULL) {
 		region_truncate(region, used);
 		return NULL;
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 5f169f09b..8807f7583 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1893,7 +1893,7 @@ vy_update(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
 	const char *old_tuple_end = old_tuple + old_size;
 	new_tuple = xrow_update_execute(request->tuple, request->tuple_end,
 					old_tuple, old_tuple_end,
-					pk->mem_format->dict, &new_size,
+					pk->mem_format, &new_size,
 					request->index_base, &column_mask);
 	if (new_tuple == NULL)
 		return -1;
@@ -2084,8 +2084,7 @@ vy_upsert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
 		return 0;
 	/* Check update operations. */
 	if (xrow_update_check_ops(request->ops, request->ops_end,
-				  pk->mem_format->dict,
-				  request->index_base) != 0) {
+				  pk->mem_format, request->index_base) != 0) {
 		return -1;
 	}
 	if (request->index_base != 0) {
@@ -2143,8 +2142,8 @@ vy_upsert(struct vy_env *env, struct vy_tx *tx, struct txn_stmt *stmt,
 
 	/* Apply upsert operations to the old tuple. */
 	new_tuple = xrow_upsert_execute(ops, ops_end, old_tuple, old_tuple_end,
-					pk->mem_format->dict, &new_size, 0,
-					false, &column_mask);
+					pk->mem_format, &new_size, 0, false,
+					&column_mask);
 	if (new_tuple == NULL)
 		return -1;
 	/*
diff --git a/src/box/vy_upsert.c b/src/box/vy_upsert.c
index 9a47dd91e..658086c45 100644
--- a/src/box/vy_upsert.c
+++ b/src/box/vy_upsert.c
@@ -58,7 +58,7 @@ vy_upsert_try_to_squash(struct tuple_format *format,
 	size_t squashed_size;
 	const char *squashed =
 		xrow_upsert_squash(old_serie, old_serie_end,
-				   new_serie, new_serie_end, format->dict,
+				   new_serie, new_serie_end, format,
 				   &squashed_size, 0);
 	if (squashed == NULL)
 		return 0;
@@ -119,7 +119,7 @@ vy_apply_upsert(struct tuple *new_stmt, struct tuple *old_stmt,
 	uint8_t old_type = vy_stmt_type(old_stmt);
 	uint64_t column_mask = COLUMN_MASK_FULL;
 	result_mp = xrow_upsert_execute(new_ops, new_ops_end, result_mp,
-					result_mp_end, format->dict, &mp_size,
+					result_mp_end, format, &mp_size,
 					0, suppress_error, &column_mask);
 	result_mp_end = result_mp + mp_size;
 	if (old_type != IPROTO_UPSERT) {
diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c
index e7a98073a..ac8c9d303 100644
--- a/src/box/xrow_update.c
+++ b/src/box/xrow_update.c
@@ -38,6 +38,7 @@
 #include "column_mask.h"
 #include "fiber.h"
 #include "xrow_update_field.h"
+#include "tuple_format.h"
 
 /**
  * UPDATE is represented by a sequence of operations, each
@@ -344,7 +345,8 @@ xrow_update_init(struct xrow_update *update, int index_base)
 }
 
 static const char *
-xrow_update_finish(struct xrow_update *update, uint32_t *p_tuple_len)
+xrow_update_finish(struct xrow_update *update, struct tuple_format *format,
+		   uint32_t *p_tuple_len)
 {
 	uint32_t tuple_len = xrow_update_array_sizeof(&update->root);
 	char *buffer = (char *) region_alloc(&fiber()->gc, tuple_len);
@@ -352,7 +354,8 @@ xrow_update_finish(struct xrow_update *update, uint32_t *p_tuple_len)
 		diag_set(OutOfMemory, tuple_len, "region_alloc", "buffer");
 		return NULL;
 	}
-	*p_tuple_len = xrow_update_array_store(&update->root, buffer,
+	*p_tuple_len = xrow_update_array_store(&update->root, &format->fields,
+					       &format->fields.root, buffer,
 					       buffer + tuple_len);
 	assert(*p_tuple_len == tuple_len);
 	return buffer;
@@ -360,17 +363,17 @@ xrow_update_finish(struct xrow_update *update, uint32_t *p_tuple_len)
 
 int
 xrow_update_check_ops(const char *expr, const char *expr_end,
-		      struct tuple_dictionary *dict, int index_base)
+		      struct tuple_format *format, int index_base)
 {
 	struct xrow_update update;
 	xrow_update_init(&update, index_base);
-	return xrow_update_read_ops(&update, expr, expr_end, dict, 0);
+	return xrow_update_read_ops(&update, expr, expr_end, format->dict, 0);
 }
 
 const char *
 xrow_update_execute(const char *expr,const char *expr_end,
 		    const char *old_data, const char *old_data_end,
-		    struct tuple_dictionary *dict, uint32_t *p_tuple_len,
+		    struct tuple_format *format, uint32_t *p_tuple_len,
 		    int index_base, uint64_t *column_mask)
 {
 	struct xrow_update update;
@@ -378,7 +381,7 @@ xrow_update_execute(const char *expr,const char *expr_end,
 	const char *header = old_data;
 	uint32_t field_count = mp_decode_array(&old_data);
 
-	if (xrow_update_read_ops(&update, expr, expr_end, dict,
+	if (xrow_update_read_ops(&update, expr, expr_end, format->dict,
 				 field_count) != 0)
 		return NULL;
 	if (xrow_update_do_ops(&update, header, old_data, old_data_end,
@@ -387,13 +390,13 @@ xrow_update_execute(const char *expr,const char *expr_end,
 	if (column_mask)
 		*column_mask = update.column_mask;
 
-	return xrow_update_finish(&update, p_tuple_len);
+	return xrow_update_finish(&update, format, p_tuple_len);
 }
 
 const char *
 xrow_upsert_execute(const char *expr,const char *expr_end,
 		    const char *old_data, const char *old_data_end,
-		    struct tuple_dictionary *dict, uint32_t *p_tuple_len,
+		    struct tuple_format *format, uint32_t *p_tuple_len,
 		    int index_base, bool suppress_error, uint64_t *column_mask)
 {
 	struct xrow_update update;
@@ -401,7 +404,7 @@ xrow_upsert_execute(const char *expr,const char *expr_end,
 	const char *header = old_data;
 	uint32_t field_count = mp_decode_array(&old_data);
 
-	if (xrow_update_read_ops(&update, expr, expr_end, dict,
+	if (xrow_update_read_ops(&update, expr, expr_end, format->dict,
 				 field_count) != 0)
 		return NULL;
 	if (xrow_upsert_do_ops(&update, header, old_data, old_data_end,
@@ -410,18 +413,19 @@ xrow_upsert_execute(const char *expr,const char *expr_end,
 	if (column_mask)
 		*column_mask = update.column_mask;
 
-	return xrow_update_finish(&update, p_tuple_len);
+	return xrow_update_finish(&update, format, p_tuple_len);
 }
 
 const char *
 xrow_upsert_squash(const char *expr1, const char *expr1_end,
 		   const char *expr2, const char *expr2_end,
-		   struct tuple_dictionary *dict, size_t *result_size,
+		   struct tuple_format *format, size_t *result_size,
 		   int index_base)
 {
 	const char *expr[2] = {expr1, expr2};
 	const char *expr_end[2] = {expr1_end, expr2_end};
 	struct xrow_update update[2];
+	struct tuple_dictionary *dict = format->dict;
 	for (int j = 0; j < 2; j++) {
 		xrow_update_init(&update[j], index_base);
 		if (xrow_update_read_ops(&update[j], expr[j], expr_end[j],
@@ -463,6 +467,10 @@ 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],
@@ -517,10 +525,13 @@ 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);
-		res_ops = mp_encode_uint(res_ops,
-					 op[0]->field_no +
-						 update[0].index_base);
-		xrow_update_op_store_arith(&res, NULL, res_ops);
+		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 += xrow_update_arg_arith_sizeof(&res.arg.arith);
 		mp_next(&expr[0]);
 		mp_next(&expr[1]);
diff --git a/src/box/xrow_update.h b/src/box/xrow_update.h
index 74e068e8f..d48c3790f 100644
--- a/src/box/xrow_update.h
+++ b/src/box/xrow_update.h
@@ -44,22 +44,22 @@ enum {
 	BOX_UPDATE_OP_CNT_MAX = 4000,
 };
 
-struct tuple_dictionary;
+struct tuple_format;
 
 int
 xrow_update_check_ops(const char *expr, const char *expr_end,
-		      struct tuple_dictionary *dict, int index_base);
+		      struct tuple_format *format, int index_base);
 
 const char *
 xrow_update_execute(const char *expr,const char *expr_end,
 		    const char *old_data, const char *old_data_end,
-		    struct tuple_dictionary *dict, uint32_t *p_new_size,
+		    struct tuple_format *format, uint32_t *p_new_size,
 		    int index_base, uint64_t *column_mask);
 
 const char *
 xrow_upsert_execute(const char *expr, const char *expr_end,
 		    const char *old_data, const char *old_data_end,
-		    struct tuple_dictionary *dict, uint32_t *p_new_size,
+		    struct tuple_format *format, uint32_t *p_new_size,
 		    int index_base, bool suppress_error,
 		    uint64_t *column_mask);
 
@@ -76,7 +76,7 @@ xrow_upsert_execute(const char *expr, const char *expr_end,
 const char *
 xrow_upsert_squash(const char *expr1, const char *expr1_end,
 		   const char *expr2, const char *expr2_end,
-		   struct tuple_dictionary *dict, size_t *result_size,
+		   struct tuple_format *format, size_t *result_size,
 		   int index_base);
 
 #if defined(__cplusplus)
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;
+		}
 	}
 	(void) total_field_count;
 	assert(xrow_update_rope_size(field->array.rope) == total_field_count);
diff --git a/src/box/xrow_update_bar.c b/src/box/xrow_update_bar.c
index 1c2d2ef99..b4a98978a 100644
--- a/src/box/xrow_update_bar.c
+++ b/src/box/xrow_update_bar.c
@@ -370,7 +370,9 @@ xrow_update_bar_sizeof(struct xrow_update_field *field)
 }
 
 uint32_t
-xrow_update_bar_store(struct xrow_update_field *field, char *out, char *out_end)
+xrow_update_bar_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_BAR);
 	(void) out_end;
@@ -431,6 +433,11 @@ xrow_update_bar_store(struct xrow_update_field *field, char *out, char *out_end)
 		return out + size - out_saved;
 	}
 	default: {
+		if (this_node != NULL) {
+			this_node = json_tree_lookup_path(
+				format_tree, this_node, field->bar.path,
+				field->bar.path_len, 0);
+		}
 		uint32_t before_point = field->bar.point - field->data;
 		const char *field_end = field->data + field->size;
 		const char *point_end =
@@ -439,7 +446,8 @@ xrow_update_bar_store(struct xrow_update_field *field, char *out, char *out_end)
 
 		memcpy(out, field->data, before_point);
 		out += before_point;
-		op->meta->store(op, field->bar.point, out);
+		op->meta->store(op, format_tree, this_node, field->bar.point,
+				out);
 		out += op->new_field_len;
 		memcpy(out, point_end, after_point);
 		return out + after_point - out_saved;
diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index 6ac29de5d..42a3ba50a 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -127,8 +127,9 @@ xrow_update_field_sizeof(struct xrow_update_field *field)
 }
 
 uint32_t
-xrow_update_field_store(struct xrow_update_field *field, char *out,
-			char *out_end)
+xrow_update_field_store(struct xrow_update_field *field,
+			struct json_tree *format_tree,
+			struct json_token *this_node, char *out, char *out_end)
 {
 	struct xrow_update_op *op;
 	uint32_t size;
@@ -141,16 +142,20 @@ xrow_update_field_store(struct xrow_update_field *field, char *out,
 		op = field->scalar.op;
 		size = op->new_field_len;
 		assert(out_end - out >= size);
-		op->meta->store(op, field->data, out);
+		op->meta->store(op, format_tree, this_node, field->data, out);
 		return size;
 	case XUPDATE_ARRAY:
-		return xrow_update_array_store(field, out, out_end);
+		return xrow_update_array_store(field, format_tree, this_node,
+					       out, out_end);
 	case XUPDATE_BAR:
-		return xrow_update_bar_store(field, out, out_end);
+		return xrow_update_bar_store(field, format_tree, this_node, out,
+					     out_end);
 	case XUPDATE_ROUTE:
-		return xrow_update_route_store(field, out, out_end);
+		return xrow_update_route_store(field, format_tree, this_node,
+					       out, out_end);
 	case XUPDATE_MAP:
-		return xrow_update_map_store(field, out, out_end);
+		return xrow_update_map_store(field, format_tree, this_node, out,
+					     out_end);
 	default:
 		unreachable();
 	}
@@ -512,15 +517,25 @@ xrow_update_op_do_splice(struct xrow_update_op *op, const char *old)
 /* {{{ store_op */
 
 static void
-xrow_update_op_store_set(struct xrow_update_op *op, const char *in, char *out)
+xrow_update_op_store_set(struct xrow_update_op *op,
+			 struct json_tree *format_tree,
+			 struct json_token *this_node, const char *in,
+			 char *out)
 {
+	(void) format_tree;
+	(void) this_node;
 	(void) in;
 	memcpy(out, op->arg.set.value, op->arg.set.length);
 }
 
 void
-xrow_update_op_store_arith(struct xrow_update_op *op, const char *in, char *out)
+xrow_update_op_store_arith(struct xrow_update_op *op,
+			   struct json_tree *format_tree,
+			   struct json_token *this_node, const char *in,
+			   char *out)
 {
+	(void) format_tree;
+	(void) this_node;
 	(void) in;
 	struct xrow_update_arg_arith *arg = &op->arg.arith;
 	switch (arg->type) {
@@ -546,16 +561,25 @@ xrow_update_op_store_arith(struct xrow_update_op *op, const char *in, char *out)
 }
 
 static void
-xrow_update_op_store_bit(struct xrow_update_op *op, const char *in, char *out)
+xrow_update_op_store_bit(struct xrow_update_op *op,
+			 struct json_tree *format_tree,
+			 struct json_token *this_node, const char *in,
+			 char *out)
 {
+	(void) format_tree;
+	(void) this_node;
 	(void) in;
 	mp_encode_uint(out, op->arg.bit.val);
 }
 
 static void
-xrow_update_op_store_splice(struct xrow_update_op *op, const char *in,
+xrow_update_op_store_splice(struct xrow_update_op *op,
+			    struct json_tree *format_tree,
+			    struct json_token *this_node, const char *in,
 			    char *out)
 {
+	(void) format_tree;
+	(void) this_node;
 	struct xrow_update_arg_splice *arg = &op->arg.splice;
 	uint32_t new_str_len = arg->offset + arg->paste_length +
 			       arg->tail_length;
diff --git a/src/box/xrow_update_field.h b/src/box/xrow_update_field.h
index 451a23024..782cb5d3f 100644
--- a/src/box/xrow_update_field.h
+++ b/src/box/xrow_update_field.h
@@ -152,7 +152,10 @@ typedef int
 		       struct xrow_update_field *field);
 
 typedef void
-(*xrow_update_op_store_f)(struct xrow_update_op *op, const char *in, char *out);
+(*xrow_update_op_store_f)(struct xrow_update_op *op,
+			  struct json_tree *format_tree,
+			  struct json_token *this_node, const char *in,
+			  char *out);
 
 /**
  * A set of functions and properties to initialize, do and store
@@ -470,8 +473,9 @@ xrow_update_field_sizeof(struct xrow_update_field *field);
 
 /** Save the updated field, including all children recursively. */
 uint32_t
-xrow_update_field_store(struct xrow_update_field *field, char *out,
-			char *out_end);
+xrow_update_field_store(struct xrow_update_field *field,
+			struct json_tree *format_tree,
+			struct json_token *this_node, char *out, char *out_end);
 
 /**
  * Generate declarations for a concrete field type: array, bar
@@ -507,7 +511,9 @@ uint32_t									\
 xrow_update_##type##_sizeof(struct xrow_update_field *field);			\
 										\
 uint32_t									\
-xrow_update_##type##_store(struct xrow_update_field *field, char *out,		\
+xrow_update_##type##_store(struct xrow_update_field *field,			\
+			   struct json_tree *format_tree,			\
+			   struct json_token *this_node, char *out,		\
 			   char *out_end);
 
 /* }}} xrow_update_field */
@@ -706,7 +712,9 @@ xrow_update_arith_make(struct xrow_update_op *op,
 		       struct xrow_update_arg_arith *ret);
 
 void
-xrow_update_op_store_arith(struct xrow_update_op *op, const char *in,
+xrow_update_op_store_arith(struct xrow_update_op *op,
+			   struct json_tree *format_tree,
+			   struct json_token *this_node, const char *in,
 			   char *out);
 
 uint32_t
diff --git a/src/box/xrow_update_map.c b/src/box/xrow_update_map.c
index b4251cc3b..ff53a9ac4 100644
--- a/src/box/xrow_update_map.c
+++ b/src/box/xrow_update_map.c
@@ -428,7 +428,9 @@ xrow_update_map_sizeof(struct xrow_update_field *field)
 }
 
 uint32_t
-xrow_update_map_store(struct xrow_update_field *field, char *out, char *out_end)
+xrow_update_map_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_MAP);
 	char *out_begin = out;
@@ -438,10 +440,32 @@ xrow_update_map_store(struct xrow_update_field *field, char *out, char *out_end)
 	 * This is the trick about saving updated keys before
 	 * others. The first cycle doesn't save unchanged tails.
 	 */
-	stailq_foreach_entry(i, &field->map.items, in_items) {
-		if (i->key != NULL) {
-			out = mp_encode_str(out, i->key, i->key_len);
-			out += xrow_update_field_store(&i->field, out, out_end);
+	if (this_node == NULL) {
+		stailq_foreach_entry(i, &field->map.items, in_items) {
+			if (i->key != NULL) {
+				out = mp_encode_str(out, i->key, i->key_len);
+				out += xrow_update_field_store(&i->field, NULL,
+							       NULL, out,
+							       out_end);
+			}
+		}
+	} else {
+		struct json_token token;
+		token.type = JSON_TOKEN_STR;
+		struct json_token *next_node;
+		stailq_foreach_entry(i, &field->map.items, in_items) {
+			if (i->key != NULL) {
+				token.str = i->key;
+				token.len = i->key_len;
+				next_node = json_tree_lookup(format_tree,
+							     this_node,
+							     &token);
+				out = mp_encode_str(out, i->key, i->key_len);
+				out += xrow_update_field_store(&i->field,
+							       format_tree,
+							       next_node, out,
+							       out_end);
+			}
 		}
 	}
 	stailq_foreach_entry(i, &field->map.items, in_items) {
diff --git a/src/box/xrow_update_route.c b/src/box/xrow_update_route.c
index 442bca9a5..122f0329e 100644
--- a/src/box/xrow_update_route.c
+++ b/src/box/xrow_update_route.c
@@ -377,14 +377,21 @@ xrow_update_route_sizeof(struct xrow_update_field *field)
 }
 
 uint32_t
-xrow_update_route_store(struct xrow_update_field *field, char *out,
-			char *out_end)
+xrow_update_route_store(struct xrow_update_field *field,
+			struct json_tree *format_tree,
+			struct json_token *this_node, char *out, char *out_end)
 {
+	if (this_node != NULL) {
+		this_node = json_tree_lookup_path(
+			format_tree, this_node, field->route.path,
+			field->route.path_len, 0);
+	}
 	char *saved_out = out;
 	int before_hop = field->route.next_hop->data - field->data;
 	memcpy(out, field->data, before_hop);
 	out += before_hop;
-	out += xrow_update_field_store(field->route.next_hop, out, out_end);
+	out += xrow_update_field_store(field->route.next_hop, format_tree,
+				       this_node, out, out_end);
 	int after_hop = before_hop + field->route.next_hop->size;
 	memcpy(out, field->data + after_hop, field->size - after_hop);
 	return out + field->size - after_hop - saved_out;
diff --git a/test/unit/column_mask.c b/test/unit/column_mask.c
index 8401a4f7f..56ce2ada7 100644
--- a/test/unit/column_mask.c
+++ b/test/unit/column_mask.c
@@ -130,7 +130,7 @@ check_update_result(const struct tuple_template *original,
 	struct region *region = &fiber()->gc;
 	const char *actual =
 		xrow_update_execute(ops, ops_end, old, old_end,
-				    box_tuple_format_default()->dict,
+				    box_tuple_format_default(),
 				    &actual_len, 1, &column_mask);
 	fail_if(actual == NULL);
 	is((int32_t)actual_len, new_end - new, "check result length");
@@ -266,7 +266,7 @@ test_paths(void)
 	uint64_t column_mask;
 	const char *result =
 		xrow_update_execute(buffer2, pos2, buffer1, pos1,
-				    box_tuple_format_default()->dict,
+				    box_tuple_format_default(),
 				    &result_size, 1, &column_mask);
 	isnt(result, NULL, "JSON update works");
 
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 3/4] tuple: allow xrow_update sizeof reserve more memory

[Vladislav Shpilevoy]

Currently xrow_update sizeof + store are used to calculate the
result tuple's size, preallocate it as a monolithic memory block,
and save the update tree into it.

Sizeof was expected to return the exact memory size needed for the
tuple. But this is not useful when result size of a field depends
on its format type, and therefore on its position in the tuple.
Because in that case sizeof would need to care about tuple format,
and walk format tree just like store does now. Or it would be
needed to save the found json tree nodes into struct
xrow_update_op during sizeof calculation. All of this would make
sizeof code more complex.

The patch makes it possible for sizeof to return the maximal
needed size. So, for example, a floating point field size now
returns size needed for encoding of a double. And then store can
either encode double or float.

Follow-up #4701
---
 src/box/xrow_update.c       |  2 +-
 src/box/xrow_update_bar.c   |  5 ++---
 src/box/xrow_update_field.c | 37 ++++++++++++++++++++++---------------
 src/box/xrow_update_field.h |  4 ++--
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c
index ac8c9d303..63f7bc3da 100644
--- a/src/box/xrow_update.c
+++ b/src/box/xrow_update.c
@@ -357,7 +357,7 @@ xrow_update_finish(struct xrow_update *update, struct tuple_format *format,
 	*p_tuple_len = xrow_update_array_store(&update->root, &format->fields,
 					       &format->fields.root, buffer,
 					       buffer + tuple_len);
-	assert(*p_tuple_len == tuple_len);
+	assert(*p_tuple_len <= tuple_len);
 	return buffer;
 }
 
diff --git a/src/box/xrow_update_bar.c b/src/box/xrow_update_bar.c
index b4a98978a..0033f0044 100644
--- a/src/box/xrow_update_bar.c
+++ b/src/box/xrow_update_bar.c
@@ -446,9 +446,8 @@ xrow_update_bar_store(struct xrow_update_field *field,
 
 		memcpy(out, field->data, before_point);
 		out += before_point;
-		op->meta->store(op, format_tree, this_node, field->bar.point,
-				out);
-		out += op->new_field_len;
+		out += op->meta->store(op, format_tree, this_node,
+				       field->bar.point, out);
 		memcpy(out, point_end, after_point);
 		return out + after_point - out_saved;
 	}
diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index 42a3ba50a..d431c22f8 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -132,7 +132,6 @@ xrow_update_field_store(struct xrow_update_field *field,
 			struct json_token *this_node, char *out, char *out_end)
 {
 	struct xrow_update_op *op;
-	uint32_t size;
 	switch(field->type) {
 	case XUPDATE_NOP:
 		assert(out_end - out >= field->size);
@@ -140,10 +139,9 @@ xrow_update_field_store(struct xrow_update_field *field,
 		return field->size;
 	case XUPDATE_SCALAR:
 		op = field->scalar.op;
-		size = op->new_field_len;
-		assert(out_end - out >= size);
-		op->meta->store(op, format_tree, this_node, field->data, out);
-		return size;
+		assert(out_end - out >= op->new_field_len);
+		return op->meta->store(op, format_tree, this_node, field->data,
+				       out);
 	case XUPDATE_ARRAY:
 		return xrow_update_array_store(field, format_tree, this_node,
 					       out, out_end);
@@ -516,7 +514,7 @@ xrow_update_op_do_splice(struct xrow_update_op *op, const char *old)
 
 /* {{{ store_op */
 
-static void
+static uint32_t
 xrow_update_op_store_set(struct xrow_update_op *op,
 			 struct json_tree *format_tree,
 			 struct json_token *this_node, const char *in,
@@ -526,9 +524,10 @@ xrow_update_op_store_set(struct xrow_update_op *op,
 	(void) this_node;
 	(void) in;
 	memcpy(out, op->arg.set.value, op->arg.set.length);
+	return op->arg.set.length;
 }
 
-void
+uint32_t
 xrow_update_op_store_arith(struct xrow_update_op *op,
 			   struct json_tree *format_tree,
 			   struct json_token *this_node, const char *in,
@@ -537,30 +536,34 @@ xrow_update_op_store_arith(struct xrow_update_op *op,
 	(void) format_tree;
 	(void) this_node;
 	(void) in;
+	char *begin = out;
 	struct xrow_update_arg_arith *arg = &op->arg.arith;
 	switch (arg->type) {
 	case XUPDATE_TYPE_INT:
 		if (int96_is_uint64(&arg->int96)) {
-			mp_encode_uint(out, int96_extract_uint64(&arg->int96));
+			out = mp_encode_uint(
+				out, int96_extract_uint64(&arg->int96));
 		} else {
 			assert(int96_is_neg_int64(&arg->int96));
-			mp_encode_int(out, int96_extract_neg_int64(&arg->int96));
+			out = mp_encode_int(
+				out, int96_extract_neg_int64( &arg->int96));
 		}
 		break;
 	case XUPDATE_TYPE_DOUBLE:
-		mp_encode_double(out, arg->dbl);
+		out = mp_encode_double(out, arg->dbl);
 		break;
 	case XUPDATE_TYPE_FLOAT:
-		mp_encode_float(out, arg->flt);
+		out = mp_encode_float(out, arg->flt);
 		break;
 	default:
 		assert(arg->type == XUPDATE_TYPE_DECIMAL);
-		mp_encode_decimal(out, &arg->dec);
+		out = mp_encode_decimal(out, &arg->dec);
 		break;
 	}
+	return out - begin;
 }
 
-static void
+static uint32_t
 xrow_update_op_store_bit(struct xrow_update_op *op,
 			 struct json_tree *format_tree,
 			 struct json_token *this_node, const char *in,
@@ -569,10 +572,11 @@ xrow_update_op_store_bit(struct xrow_update_op *op,
 	(void) format_tree;
 	(void) this_node;
 	(void) in;
-	mp_encode_uint(out, op->arg.bit.val);
+	char *end = mp_encode_uint(out, op->arg.bit.val);
+	return end - out;
 }
 
-static void
+static uint32_t
 xrow_update_op_store_splice(struct xrow_update_op *op,
 			    struct json_tree *format_tree,
 			    struct json_token *this_node, const char *in,
@@ -583,6 +587,7 @@ xrow_update_op_store_splice(struct xrow_update_op *op,
 	struct xrow_update_arg_splice *arg = &op->arg.splice;
 	uint32_t new_str_len = arg->offset + arg->paste_length +
 			       arg->tail_length;
+	char *begin = out;
 	(void) mp_decode_strl(&in);
 	out = mp_encode_strl(out, new_str_len);
 	/* Copy field head. */
@@ -593,6 +598,8 @@ xrow_update_op_store_splice(struct xrow_update_op *op,
 	out = out + arg->paste_length;
 	/* Copy tail. */
 	memcpy(out, in + arg->tail_offset, arg->tail_length);
+	out = out + arg->tail_length;
+	return out - begin;
 }
 
 /* }}} store_op */
diff --git a/src/box/xrow_update_field.h b/src/box/xrow_update_field.h
index 782cb5d3f..5a8a79881 100644
--- a/src/box/xrow_update_field.h
+++ b/src/box/xrow_update_field.h
@@ -151,7 +151,7 @@ typedef int
 (*xrow_update_op_do_f)(struct xrow_update_op *op,
 		       struct xrow_update_field *field);
 
-typedef void
+typedef uint32_t
 (*xrow_update_op_store_f)(struct xrow_update_op *op,
 			  struct json_tree *format_tree,
 			  struct json_token *this_node, const char *in,
@@ -711,7 +711,7 @@ xrow_update_arith_make(struct xrow_update_op *op,
 		       struct xrow_update_arg_arith arg,
 		       struct xrow_update_arg_arith *ret);
 
-void
+uint32_t
 xrow_update_op_store_arith(struct xrow_update_op *op,
 			   struct json_tree *format_tree,
 			   struct json_token *this_node, const char *in,
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v2 4/4] tuple: use field type in update of a float/double

[Vladislav Shpilevoy]

There was a bug that float +/- float could result into infinity
even if result fits a double. It was fixed by storing double or
float depending on a result value. But it didn't take result field
type into account. That led to a bug when a double field +/- a
value fit the float range, and could be stored as float resulting
into an error at attempt to create a tuple.

Now if a field type is double in the tuple format, it will store
double always, even if it fits the float range.

Follow-up #4701
---
 src/box/xrow_update_field.c | 13 ++++++++++---
 test/box/update.result      | 38 +++++++++++++++++++++++++++++++++++++
 test/box/update.test.lua    | 17 +++++++++++++++++
 3 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index d431c22f8..cc64cf955 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -352,9 +352,8 @@ xrow_update_arg_arith_sizeof(const struct xrow_update_arg_arith *arg)
 		}
 		break;
 	case XUPDATE_TYPE_DOUBLE:
-		return mp_sizeof_double(arg->dbl);
 	case XUPDATE_TYPE_FLOAT:
-		return mp_sizeof_float(arg->flt);
+		return mp_sizeof_double(arg->dbl);
 	default:
 		assert(arg->type == XUPDATE_TYPE_DECIMAL);
 		return mp_sizeof_decimal(&arg->dec);
@@ -534,7 +533,6 @@ xrow_update_op_store_arith(struct xrow_update_op *op,
 			   char *out)
 {
 	(void) format_tree;
-	(void) this_node;
 	(void) in;
 	char *begin = out;
 	struct xrow_update_arg_arith *arg = &op->arg.arith;
@@ -553,6 +551,15 @@ xrow_update_op_store_arith(struct xrow_update_op *op,
 		out = mp_encode_double(out, arg->dbl);
 		break;
 	case XUPDATE_TYPE_FLOAT:
+		if (this_node != NULL) {
+			enum field_type type =
+				json_tree_entry(this_node, struct tuple_field,
+						token)->type;
+			if (type == FIELD_TYPE_DOUBLE) {
+				out = mp_encode_double(out, arg->flt);
+				break;
+			}
+		}
 		out = mp_encode_float(out, arg->flt);
 		break;
 	default:
diff --git a/test/box/update.result b/test/box/update.result
index dfbd8714f..04866006a 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -1712,3 +1712,41 @@ err
 ---
 - null
 ...
+-- Check that if a field has 'double' field type, it won't use
+-- float, even when the value fits float range.
+s = box.schema.create_space('test', {                                           \
+    format = {{'field1', 'unsigned'}, {'field2', 'double'}, {'field3', 'any'}}  \
+})
+---
+...
+_ = s:create_index('pk')
+---
+...
+dbl1 = ffi.cast('double', 1)
+---
+...
+_ = s:replace{1, dbl1, 1}
+---
+...
+s:update({1}, {{'+', 2, dbl1}})
+---
+- [1, 2, 1]
+...
+_ = s:delete{1}
+---
+...
+-- Check deep fields.
+_ = s:create_index('deep_sk', {'field3', 'double', path = '[1].key1.key2[2]'})
+---
+- error: Illegal parameters, unexpected option '1'
+...
+_ = s:replace{1, dbl1, {{key1 = {key2 = {1, dbl1}}}}}
+---
+...
+s:update({1}, {{'+', 'field3[1].key1.key2[2]', dbl1}})
+---
+- [1, 1, [{'key1': {'key2': [1, 2]}}]]
+...
+s:drop()
+---
+...
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index 74f9c62e2..d52be4f3b 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -642,3 +642,20 @@ for i, test in pairs(tests) do
     end                                                                         \
 end
 err
+
+-- Check that if a field has 'double' field type, it won't use
+-- float, even when the value fits float range.
+s = box.schema.create_space('test', {                                           \
+    format = {{'field1', 'unsigned'}, {'field2', 'double'}, {'field3', 'any'}}  \
+})
+_ = s:create_index('pk')
+dbl1 = ffi.cast('double', 1)
+_ = s:replace{1, dbl1, 1}
+s:update({1}, {{'+', 2, dbl1}})
+_ = s:delete{1}
+-- Check deep fields.
+_ = s:create_index('deep_sk', {'field3', 'double', path = '[1].key1.key2[2]'})
+_ = s:replace{1, dbl1, {{key1 = {key2 = {1, dbl1}}}}}
+s:update({1}, {{'+', 'field3[1].key1.key2[2]', dbl1}})
+
+s:drop()
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH v2 0/4] Don't truncate float in update

[Konstantin Osipov]

* Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/02/05 01:53]:
> The patchset fixes float value truncation in
> tuple/space:update/upsert. First commit fixes the actual issue,
> but introduces another bug that a float value can be stored into a
> field having double type in its format.

Thanks for CCing. While I find the general idea of the series quite
nice, I don't know why it's a priority.

Did anyone actually complain?

PS Would be best if you don't Cc/To me with the entire series next
time, but @mention me on the list if you would like to get my review. I am
reading all patches on the list anyway, no need to copy them to my inbox.

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 0/4] Don't truncate float in update

[Vladislav Shpilevoy]

Hi!

On 05/02/2020 12:09, Konstantin Osipov wrote:
> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [20/02/05 01:53]:
>> The patchset fixes float value truncation in
>> tuple/space:update/upsert. First commit fixes the actual issue,
>> but introduces another bug that a float value can be stored into a
>> field having double type in its format.
> 
> Thanks for CCing. While I find the general idea of the series quite
> nice, I don't know why it's a priority.

It was assigned to a milestone, and the code still is in my
head's "cache" after JSON updates, so I decided to fix it.

> Did anyone actually complain?

Nobody so far. Just me.

> PS Would be best if you don't Cc/To me with the entire series next
> time, but @mention me on the list if you would like to get my review. I am
> reading all patches on the list anyway, no need to copy them to my inbox.

Ok, will do that next time.

Re: [Tarantool-patches] [PATCH v2 1/4] tuple: don't truncate float in :update()

[Nikita Pettik]

On 04 Feb 23:53, Vladislav Shpilevoy wrote:
> diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
> index 7c0f5fb5e..6ac29de5d 100644
> --- a/src/box/xrow_update_field.c
> +++ b/src/box/xrow_update_field.c
> @@ -32,6 +32,8 @@
>  #include "tuple_format.h"
>  #include "mp_extension_types.h"
>  
> +#include <float.h>
> +
>  /* {{{ Error helpers. */
>  
>  /** Take a string identifier of a field being updated by @a op. */
> @@ -400,13 +402,15 @@ xrow_update_arith_make(struct xrow_update_op *op,
>  			unreachable();
>  			break;
>  		}

Patch is ok, but I'd add a brief comment here like:

If value fits into float, then store it as a float;
store it as double otherwise.

Up to you.

> -		if (lowest_type == XUPDATE_TYPE_DOUBLE) {
> +		float fc = (float) c;
> +		if ((lowest_type == XUPDATE_TYPE_DOUBLE && c != (double) fc) ||
> +		    (lowest_type == XUPDATE_TYPE_FLOAT &&
> +		     (c >= FLT_MAX || c <= -FLT_MAX))) {
>  			ret->type = XUPDATE_TYPE_DOUBLE;
>  			ret->dbl = c;
>  		} else {
> -			assert(lowest_type == XUPDATE_TYPE_FLOAT);
>  			ret->type = XUPDATE_TYPE_FLOAT;
> -			ret->flt = (float)c;
> +			ret->flt = fc;
>  		}
>  	} else {
>  		decimal_t a, b, c;

Re: [Tarantool-patches] [PATCH v2 3/4] tuple: allow xrow_update sizeof reserve more memory

[Nikita Pettik]

On 04 Feb 23:53, Vladislav Shpilevoy wrote:
> 

I am not familiar with update/json code much, but patch looks ok to
a newbie like me :)

Re: [Tarantool-patches] [PATCH v2 4/4] tuple: use field type in update of a float/double

[Nikita Pettik]

On 04 Feb 23:53, Vladislav Shpilevoy wrote:

LGTM

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

[Nikita Pettik]

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:

diff --git a/src/box/xrow_update_array.c b/src/box/xrow_update_array.c
index 717466be7..c8ca886ae 100644
--- a/src/box/xrow_update_array.c
+++ b/src/box/xrow_update_array.c
@@ -271,38 +271,26 @@ xrow_update_array_store(struct xrow_update_field *field,
        xrow_update_rope_iter_create(&it, field->array.rope);
        struct xrow_update_rope_node *node = xrow_update_rope_iter_start(&it);
        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;
+       struct json_token *next_node = NULL;
+       struct json_token token;
+       token.type = JSON_TOKEN_NUM;
+       token.num = 0;
+       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);
+               if (this_node != NULL) {
+                       next_node = json_tree_lookup(format_tree, this_node,
+                                                    &token);
                        token.num += field_count;
-                       total_field_count += field_count;
                }
+               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;
+               total_field_count += field_count;
        }

Re: [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]);
================================================================================

Re: [Tarantool-patches] [PATCH v2 1/4] tuple: don't truncate float in :update()

[Vladislav Shpilevoy]

Hi! Thanks for the review!

On 10/02/2020 15:41, Nikita Pettik wrote:
> On 04 Feb 23:53, Vladislav Shpilevoy wrote:
>> diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
>> index 7c0f5fb5e..6ac29de5d 100644
>> --- a/src/box/xrow_update_field.c
>> +++ b/src/box/xrow_update_field.c
>> @@ -32,6 +32,8 @@
>>  #include "tuple_format.h"
>>  #include "mp_extension_types.h"
>>  
>> +#include <float.h>
>> +
>>  /* {{{ Error helpers. */
>>  
>>  /** Take a string identifier of a field being updated by @a op. */
>> @@ -400,13 +402,15 @@ xrow_update_arith_make(struct xrow_update_op *op,
>>  			unreachable();
>>  			break;
>>  		}
> 
> Patch is ok, but I'd add a brief comment here like:
> 
> If value fits into float, then store it as a float;
> store it as double otherwise.
> 
> Up to you.

Added a comment:

================================================================================

diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index 6ac29de5d..0540e8c3e 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -403,6 +403,12 @@ xrow_update_arith_make(struct xrow_update_op *op,
 			break;
 		}
 		float fc = (float) c;
+		/*
+		 * If fits into float - store as a float.
+		 * Otherwise as a double. Fits means, that the
+		 * value precision is not truncated by float, and
+		 * its value is inside the float range.
+		 */
 		if ((lowest_type == XUPDATE_TYPE_DOUBLE && c != (double) fc) ||
 		    (lowest_type == XUPDATE_TYPE_FLOAT &&
 		     (c >= FLT_MAX || c <= -FLT_MAX))) {

Re: [Tarantool-patches] [PATCH v2 4/4] tuple: use field type in update of a float/double

[Vladislav Shpilevoy]

Thanks for the review!

The test was broken here. I fixed it and added a new test for
update operations, having a common prefix. To test that
xrow_update_route.c uses correct index base for JSON indexes.

================================================================================
diff --git a/test/box/update.result b/test/box/update.result
index 04866006a..65f14ea86 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -1736,9 +1736,10 @@ _ = s:delete{1}
 ---
 ...
 -- Check deep fields.
-_ = s:create_index('deep_sk', {'field3', 'double', path = '[1].key1.key2[2]'})
+_ = s:create_index('deep_sk', {                                                 \
+    parts = {{'field3', 'double', path = '[1].key1.key2[2]'}}                   \
+})
 ---
-- error: Illegal parameters, unexpected option '1'
 ...
 _ = s:replace{1, dbl1, {{key1 = {key2 = {1, dbl1}}}}}
 ---
@@ -1747,6 +1748,13 @@ s:update({1}, {{'+', 'field3[1].key1.key2[2]', dbl1}})
 ---
 - [1, 1, [{'key1': {'key2': [1, 2]}}]]
 ...
+-- Check update operations with common prefix.
+s:update({1}, {                                                                 \
+    {'=', 'field3[1].key1.key3', 100}, {'+', 'field3[1].key1.key2[2]', dbl1}    \
+})
+---
+- [1, 1, [{'key1': {'key2': [1, 3], 'key3': 100}}]]
+...
 s:drop()
 ---
 ...
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index d52be4f3b..d4a2f455c 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -654,8 +654,14 @@ _ = s:replace{1, dbl1, 1}
 s:update({1}, {{'+', 2, dbl1}})
 _ = s:delete{1}
 -- Check deep fields.
-_ = s:create_index('deep_sk', {'field3', 'double', path = '[1].key1.key2[2]'})
+_ = s:create_index('deep_sk', {                                                 \
+    parts = {{'field3', 'double', path = '[1].key1.key2[2]'}}                   \
+})
 _ = s:replace{1, dbl1, {{key1 = {key2 = {1, dbl1}}}}}
 s:update({1}, {{'+', 'field3[1].key1.key2[2]', dbl1}})
+-- Check update operations with common prefix.
+s:update({1}, {                                                                 \
+    {'=', 'field3[1].key1.key3', 100}, {'+', 'field3[1].key1.key2[2]', dbl1}    \
+})
 
 s:drop()

Re: [Tarantool-patches] [PATCH v2 1/4] tuple: don't truncate float in :update()

[Nikita Pettik]

On 10 Feb 22:18, Vladislav Shpilevoy wrote:
> Hi! Thanks for the review!
>

Patch-set LGTM.
 

Re: [Tarantool-patches] [PATCH v2 0/4] Don't truncate float in update

[Kirill Yukhin]

Hello,

On 04 фев 23:53, Vladislav Shpilevoy wrote:
> The patchset fixes float value truncation in
> tuple/space:update/upsert. First commit fixes the actual issue,
> but introduces another bug that a float value can be stored into a
> field having double type in its format.
> 
> The rest 3 commits fix this new bug.
> 
> However, there is an alternative how these 3 could be done. My
> solution is that I calculate the result types in
> xrow_update_*_store() functions. Because of that I don't know the
> types during xrow_update_*_sizeof(), and it needs to return the
> maximal possible size instead of exact size. The alternative is to
> calculate types during sizeof. It would allow to return the exact
> size. But it also would complicate the code. I can implement this
> option if it looks better.
> 
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4701-update-float-truncate
> Issue: https://github.com/tarantool/tarantool/issues/4701

LGTM. I've checked your patchset into 2.3 and master.

--
Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2 0/4] Don't truncate float in update

[Nikita Pettik]

On 20 Feb 09:15, Kirill Yukhin wrote:
> Hello,
> 
> On 04 фев 23:53, Vladislav Shpilevoy wrote:
> > The patchset fixes float value truncation in
> > tuple/space:update/upsert. First commit fixes the actual issue,
> > but introduces another bug that a float value can be stored into a
> > field having double type in its format.
> > 
> > The rest 3 commits fix this new bug.
> > 
> > However, there is an alternative how these 3 could be done. My
> > solution is that I calculate the result types in
> > xrow_update_*_store() functions. Because of that I don't know the
> > types during xrow_update_*_sizeof(), and it needs to return the
> > maximal possible size instead of exact size. The alternative is to
> > calculate types during sizeof. It would allow to return the exact
> > size. But it also would complicate the code. I can implement this
> > option if it looks better.
> > 
> > Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4701-update-float-truncate
> > Issue: https://github.com/tarantool/tarantool/issues/4701
> 
> LGTM. I've checked your patchset into 2.3 and master.

It seems you forgot to update changelog. Vlad, could you please
paste it here https://github.com/tarantool/tarantool/releases/tag/untagged-2929e826f0a4c08b1c81
 
> --
> Regards, Kirill Yukhin

Re: [Tarantool-patches] [PATCH v2 0/4] Don't truncate float in update

[Vladislav Shpilevoy]

Hi!

Why did you decide not to push it into 2.2 and 1.10? This is
a bug, and it exists in these versions too.

On 20/02/2020 07:15, Kirill Yukhin wrote:
> Hello,
> 
> On 04 фев 23:53, Vladislav Shpilevoy wrote:
>> The patchset fixes float value truncation in
>> tuple/space:update/upsert. First commit fixes the actual issue,
>> but introduces another bug that a float value can be stored into a
>> field having double type in its format.
>>
>> The rest 3 commits fix this new bug.
>>
>> However, there is an alternative how these 3 could be done. My
>> solution is that I calculate the result types in
>> xrow_update_*_store() functions. Because of that I don't know the
>> types during xrow_update_*_sizeof(), and it needs to return the
>> maximal possible size instead of exact size. The alternative is to
>> calculate types during sizeof. It would allow to return the exact
>> size. But it also would complicate the code. I can implement this
>> option if it looks better.
>>
>> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-4701-update-float-truncate
>> Issue: https://github.com/tarantool/tarantool/issues/4701
> 
> LGTM. I've checked your patchset into 2.3 and master.
> 
> --
> Regards, Kirill Yukhin
>