Skip to content

box: fix update operations on the fixed-size floating-point fields#10779

Merged
locker merged 1 commit intotarantool:masterfrom
Gumix:iverbin/gh-9929-error-on-update-of-a-float64-field-adding-another-float64
Nov 5, 2024
Merged

box: fix update operations on the fixed-size floating-point fields#10779
locker merged 1 commit intotarantool:masterfrom
Gumix:iverbin/gh-9929-error-on-update-of-a-float64-field-adding-another-float64

Conversation

@Gumix
Copy link
Contributor

@Gumix Gumix commented Nov 1, 2024

Currently for floating-point arithmetic operations the smallest type that can represent the resulting value is chosen (MP_FLOAT vs. MP_DOUBLE).
This doesn't work for fixed-size field types (float32 and float64) that require the data to be encoded only to the corresponding type.

Closes #9929
Follow-up #9548

@Gumix Gumix requested a review from a team as a code owner November 1, 2024 15:23
@Gumix Gumix requested a review from nshy November 1, 2024 15:24
@coveralls
Copy link

coveralls commented Nov 1, 2024

Coverage Status

coverage: 87.282%. remained the same
when pulling 06ccd09 on Gumix:iverbin/gh-9929-error-on-update-of-a-float64-field-adding-another-float64
into 54afab1
on tarantool:master
.

@Gumix Gumix requested a review from locker November 1, 2024 15:52
@Gumix Gumix assigned locker and Gumix and unassigned locker and nshy Nov 1, 2024
@Gumix
Copy link
Contributor Author

Gumix commented Nov 1, 2024

Alternative fix:

diff --git a/src/box/field_def.c b/src/box/field_def.c
index 24b213154d..7826fb356b 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -91,8 +91,8 @@ const uint32_t field_mp_type[] = {
        /* [FIELD_TYPE_UINT32]   =  */ 1U << MP_UINT,
        /* [FIELD_TYPE_INT64]    =  */ (1U << MP_UINT) | (1U << MP_INT),
        /* [FIELD_TYPE_UINT64]   =  */ 1U << MP_UINT,
-       /* [FIELD_TYPE_FLOAT32]  =  */ 1U << MP_FLOAT,
-       /* [FIELD_TYPE_FLOAT64]  =  */ 1U << MP_DOUBLE,
+       /* [FIELD_TYPE_FLOAT32]  =  */ (1U << MP_FLOAT) | (1U << MP_DOUBLE),
+       /* [FIELD_TYPE_FLOAT64]  =  */ (1U << MP_FLOAT) | (1U << MP_DOUBLE),
 };

 static_assert(lengthof(field_mp_type) == field_type_MAX,
diff --git a/test/engine-luatest/gh_9548_fixed_size_numeric_types_test.lua b/test/engine-luatest/gh_9548_fixed_size_numeric_types_test.lua
index 685b7f504f..899673944d 100644
--- a/test/engine-luatest/gh_9548_fixed_size_numeric_types_test.lua
+++ b/test/engine-luatest/gh_9548_fixed_size_numeric_types_test.lua
@@ -153,29 +153,6 @@ g1.test_fixed_size_signed_json = function(cg)
     test_fixed_size_signed(cg, {has_indexed_json_path = true})
 end

--- Test fixed-size floating point types. There is no value range check.
--- float32 can store only MP_FLOAT, and float64 can store only MP_DOUBLE.
-g1.test_fixed_size_float = function(cg)
-    cg.server:exec(function()
-        local ffi = require('ffi')
-        local s = box.space.test
-        s:format({{name = 'pk',  type = 'unsigned'},
-                  {name = 'f32', type = 'float32' },
-                  {name = 'f64', type = 'float64' }})
-        local float32 = ffi.cast('float', 12.34)
-        local float64 = ffi.cast('double', 56.78)
-        s:insert({0, float32, float64})
-        t.assert_error_msg_equals(
-            "Tuple field 2 (f32) type does not match one required by " ..
-            "operation: expected float32, got double",
-            s.insert, s, {1, float64, float64})
-        t.assert_error_msg_equals(
-            "Tuple field 3 (f64) type does not match one required by " ..
-            "operation: expected float64, got float",
-            s.insert, s, {1, float32, float32})
-    end)
-end
-
 -- Check that value range check works with nullable fields.
 g1.test_nullable = function(cg)
     cg.server:exec(function()
@@ -278,27 +255,6 @@ g1.test_format_change = function(cg)
                 allow = {'int64'},
                 forbid = {'int8', 'int16', 'int32'},
                 error = "exceeds the supported range for type"
-            }, {
-                type = 'double',
-                value = ffi.cast('float', 123.456),
-                allow = {'float32'},
-                forbid = {'float64'},
-                error = "type does not match one required by operation: " ..
-                        "expected float64, got float"
-            }, {
-                type = 'double',
-                value = ffi.cast('double', 123.456),
-                allow = {'float64'},
-                forbid = {'float32'},
-                error = "type does not match one required by operation: " ..
-                        "expected float32, got double"
-            }, {
-                type = 'float32',
-                value = ffi.cast('float', 123.456),
-                allow = {'double', 'any'},
-                forbid = {'float64'},
-                error = "type does not match one required by operation: " ..
-                        "expected float64, got float"
             }
         }

@Gumix Gumix force-pushed the iverbin/gh-9929-error-on-update-of-a-float64-field-adding-another-float64 branch from 14675d1 to f1b11ee Compare November 2, 2024 14:42
@Gumix
Copy link
Contributor Author

Gumix commented Nov 2, 2024

Added testcase:

        s:replace({0, ffi.cast('float', -3.40e38),
                      ffi.cast('double', -3.40e38)})
        s:replace({1, ffi.cast('float', 3.40e38),
                      ffi.cast('double', 3.40e38)})
        s:update(0, {{'-', 'f32', ffi.cast('float', 3.40e38)},
                     {'-', 'f64', ffi.cast('double', 3.40e38)}})
        s:update(1, {{'+', 'f32', ffi.cast('float', 3.40e38)},
                     {'+', 'f64', ffi.cast('double', 3.40e38)}})
        t.assert_equals(s:select(), {{0, -math.huge, -6.8e38},
                                     {1, math.huge, 6.8e38}})

@Gumix Gumix assigned locker and nshy and unassigned Gumix Nov 2, 2024
@locker locker removed their assignment Nov 2, 2024
@nshy nshy assigned Gumix and unassigned nshy Nov 2, 2024
Currently for floating-point arithmetic operations the smallest type that
can represent the resulting value is chosen (MP_FLOAT vs. MP_DOUBLE). This
doesn't work for fixed-size field types (float32 and float64) that require
the data to be encoded only to the corresponding type.

Closes tarantool#9929
Follow-up tarantool#9548

NO_DOC=bugfix
@Gumix Gumix force-pushed the iverbin/gh-9929-error-on-update-of-a-float64-field-adding-another-float64 branch from f1b11ee to 06ccd09 Compare November 5, 2024 14:48
@Gumix Gumix added the full-ci Enables all tests for a pull request label Nov 5, 2024
@Gumix Gumix assigned locker and unassigned Gumix Nov 5, 2024
@locker locker merged commit a6aa249 into tarantool:master Nov 5, 2024
@locker
Copy link
Member

locker commented Nov 5, 2024

Cherry-picked to 3.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full-ci Enables all tests for a pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error on update of a float64 field adding another float64

5 participants