fix(expr): round decimal divide result when return type is Int512 (#7035

) (#7042) * This is an automated cherry-pick of #7035 Signed-off-by: ti-chi-bot <[email protected]> * Update divide.cpp * Update gtest_arithmetic_functions.cpp * Update gtest_arithmetic_functions.cpp * Update divide.cpp * Update gtest_arithmetic_functions.cpp * Merge missed fixes for decimal divide Signed-off-by: yibin <[email protected]> --------- Signed-off-by: ti-chi-bot <[email protected]> Signed-off-by: yibin <[email protected]> Co-authored-by: Zhi Qi <[email protected]> Co-authored-by: xufei <[email protected]> Co-authored-by: yibin <[email protected]>
pingcap · Mar 15, 2023 · 7883d6f · 7883d6f
1 parent 1b77c54
commit 7883d6f
Show file tree

Hide file tree

Showing 3 changed files with 178 additions and 6 deletions.
diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp
@@ -60,9 +60,36 @@ struct TiDBDivideFloatingImpl<A, B, false>
     using ResultType = typename NumberTraits::ResultOfFloatingPointDivision<A, B>::Type;
 
     template <typename Result = ResultType>
-    static Result apply(A a, B b)
+    static Result apply(A x, B d)
     {
-        return static_cast<Result>(a) / b;
+        /// ref https://github.com/pingcap/tiflash/issues/6462
+        /// For division of Decimal/Decimal or Int/Decimal or Decimal/Int, we should round the result to make compatible with TiDB.
+        /// basically refer to https://stackoverflow.com/a/71634489
+        if constexpr (std::is_integral_v<Result> || std::is_same_v<Result, Int256> || std::is_same_v<Result, Int512>)
+        {
+            /// 1. do division first, get the quotient and mod, todo:(perf) find a unified `divmod` function to speed up this.
+            Result quotient = x / d;
+            Result mod = x % d;
+            /// 2. get the half of divisor, which is threshold to decide whether to round up or down.
+            /// note: don't directly use bit operation here, it may cause unexpected result.
+            Result half = (d / 2) + (d % 2);
+
+            /// 3. compare the abstract values of mod and half, if mod >= half, then round up.
+            Result abs_m = mod < 0 ? -mod : mod;
+            Result abs_h = half < 0 ? -half : half;
+            if (abs_m >= abs_h)
+            {
+                /// 4. now we need to round up, i.e., add 1 to the quotient's absolute value.
+                ///    if the signs of dividend and divisor are the same, then the quotient should be positive, otherwise negative.
+                if ((x < 0) == (d < 0)) // same_sign, i.e., quotient >= 0
+                    quotient = quotient + 1;
+                else
+                    quotient = quotient - 1;
+            }
+            return quotient;
+        }
+        else
+            return static_cast<Result>(x) / d;
     }
     template <typename Result = ResultType>
     static Result apply(A a, B b, UInt8 & res_null)
@@ -75,7 +102,7 @@ struct TiDBDivideFloatingImpl<A, B, false>
             res_null = 1;
             return static_cast<Result>(0);
         }
-        return static_cast<Result>(a) / b;
+        return apply<Result>(a, b);
     }
 };
 
@@ -102,7 +129,7 @@ struct TiDBDivideFloatingImpl<A, B, true>
             res_null = 1;
             return static_cast<Result>(0);
         }
-        return static_cast<Result>(a) / static_cast<Result>(b);
+        return apply<Result>(a, b);
     }
 };
 
@@ -332,4 +359,4 @@ void registerFunctionDivideIntegralOrZero(FunctionFactory & factory)
     factory.registerFunction<FunctionDivideIntegralOrZero>();
 }
 
-} // namespace DB
+} // namespace DB
diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp
@@ -19,7 +19,9 @@
 #include <Interpreters/Context.h>
 #include <TestUtils/FunctionTestUtils.h>
 #include <TestUtils/TiFlashTestBasic.h>
+#include <gtest/gtest.h>
 
+#include <Functions/divide.cpp>
 #include <string>
 #include <unordered_map>
 #include <vector>
@@ -103,6 +105,142 @@ class TestBinaryArithmeticFunctions : public DB::tests::FunctionTest
     }
 };
 
+template <typename TYPE>
+void doTiDBDivideDecimalRoundInternalTest()
+{
+    auto apply = static_cast<TYPE (*)(TYPE, TYPE)>(&TiDBDivideFloatingImpl<TYPE, TYPE, false>::apply);
+
+    constexpr TYPE max = std::numeric_limits<TYPE>::max();
+    // note: Int256's min is not equal to -max-1
+    // according to https://www.boost.org/doc/libs/1_60_0/libs/multiprecision/doc/html/boost_multiprecision/tut/ints/cpp_int.html
+    constexpr TYPE min = std::numeric_limits<TYPE>::min();
+
+    // clang-format off
+    const std::vector<std::array<TYPE, 3>> cases = {
+        {1, 2, 1}, {1, -2, -1}, {-1, 2, -1}, {-1, -2, 1},
+
+        {0, 3, 0}, {0, -3, 0}, {0, 3, 0}, {0, -3, 0},
+        {1, 3, 0}, {1, -3, 0}, {-1, 3, 0}, {-1, -3, 0},
+        {2, 3, 1}, {2, -3, -1}, {-2, 3, -1}, {-2, -3, 1},
+        {3, 3, 1}, {3, -3, -1}, {-3, 3, -1}, {-3, -3, 1},
+        {4, 3, 1}, {4, -3, -1}, {-4, 3, -1}, {-4, -3, 1},
+        {5, 3, 2}, {5, -3, -2}, {-5, 3, -2}, {-5, -3, 2},
+
+        // ±max as divisor
+        {0, max, 0}, {max/2-1, max, 0}, {max/2, max, 0}, {max/2+1, max, 1}, {max-1, max, 1}, {max, max, 1},
+        {-1, max, 0}, {-max/2+1, max, 0},  {-max/2, max, 0}, {-max/2-1, max, -1}, {-max+1, max, -1}, {-max, max, -1}, {min, max, -1},
+        {0, -max, 0}, {max/2-1, -max, 0}, {max/2, -max, 0}, {max/2+1, -max, -1}, {max-1, -max, -1}, {max, -max, -1},
+        {-1, -max, 0}, {-max/2+1, -max, 0},  {-max/2, -max, 0}, {-max/2-1, -max, 1}, {-max+1, -max, 1}, {-max, -max, 1}, {min, -max, 1},
+
+        // ±max as dividend
+        {max, 1, max}, {max, 2, max/2+1}, {max, max/2-1, 2}, {max, max/2, 2}, {max, max/2+1, 2}, {max, max-1, 1},
+        {max, -1, -max}, {max, -2, -max/2-1}, {max, -max/2+1, -2}, {max, -max/2, -2}, {max, -max/2-1, -2}, {max, -max+1, -1},
+        {-max, 1, -max}, {-max, 2, -max/2-1}, {-max, max/2+1, -2}, {-max, max/2, -2}, {-max, max/2-1, -2}, {-max, max-1, -1},
+        {-max, -1, max}, {-max, -2, max/2+1}, {-max, -max/2-1, 2}, {-max, -max/2, 2}, {-max, -max/2+1, 2}, {-max, -max+1, 1},
+    };
+    // clang-format on
+
+    for (const auto & expect : cases)
+    {
+        std::array<TYPE, 3> actual = {expect[0], expect[1], apply(expect[0], expect[1])};
+        ASSERT_EQ(expect, actual);
+    }
+}
+
+TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRoundInternal)
+try
+{
+    doTiDBDivideDecimalRoundInternalTest<Int32>();
+    doTiDBDivideDecimalRoundInternalTest<Int64>();
+    doTiDBDivideDecimalRoundInternalTest<Int128>();
+    doTiDBDivideDecimalRoundInternalTest<Int256>();
+    doTiDBDivideDecimalRoundInternalTest<Int512>();
+}
+CATCH
+
+TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRound)
+try
+{
+    const String func_name = "tidbDivide";
+
+    // decimal32
+    {
+        // int and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal64>>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}),
+            executeFunction(
+                func_name,
+                createColumn<Int32>({1, 1, 1, 1, 1}),
+                createColumn<Decimal32>(std::make_tuple(20, 4), {DecimalField32(100000000, 4), DecimalField32(100010000, 4), DecimalField32(199990000, 4), DecimalField32(200000000, 4), DecimalField32(200010000, 4)})));
+
+        // decimal and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal128>>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}),
+            executeFunction(
+                func_name,
+                createColumn<Decimal32>(std::make_tuple(18, 4), {DecimalField32(10000, 4), DecimalField32(10000, 4), DecimalField32(10000, 4), DecimalField32(10000, 4), DecimalField32(10000, 4)}),
+                createColumn<Decimal32>(std::make_tuple(18, 4), {DecimalField32(100000000, 4), DecimalField32(100010000, 4), DecimalField32(199990000, 4), DecimalField32(200000000, 4), DecimalField32(200010000, 4)})));
+    }
+
+    // decimal64
+    {
+        // int and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal64>>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}),
+            executeFunction(
+                func_name,
+                createColumn<Int32>({1, 1, 1, 1, 1}),
+                createColumn<Decimal64>(std::make_tuple(20, 4), {DecimalField64(100000000, 4), DecimalField64(100010000, 4), DecimalField64(199990000, 4), DecimalField64(200000000, 4), DecimalField64(200010000, 4)})));
+
+        // decimal and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal128>>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}),
+            executeFunction(
+                func_name,
+                createColumn<Decimal64>(std::make_tuple(18, 4), {DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4)}),
+                createColumn<Decimal64>(std::make_tuple(18, 4), {DecimalField64(100000000, 4), DecimalField64(100010000, 4), DecimalField64(199990000, 4), DecimalField64(200000000, 4), DecimalField64(200010000, 4)})));
+    }
+
+    // decimal128
+    {
+        // int and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal64>>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}),
+            executeFunction(
+                func_name,
+                createColumn<Int32>({1, 1, 1, 1, 1}),
+                createColumn<Decimal128>(std::make_tuple(20, 4), {DecimalField128(100000000, 4), DecimalField128(100010000, 4), DecimalField128(199990000, 4), DecimalField128(200000000, 4), DecimalField128(200010000, 4)})));
+
+        // decimal and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal128>>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}),
+            executeFunction(
+                func_name,
+                createColumn<Decimal128>(std::make_tuple(18, 4), {DecimalField128(10000, 4), DecimalField128(10000, 4), DecimalField128(10000, 4), DecimalField128(10000, 4), DecimalField128(10000, 4)}),
+                createColumn<Decimal128>(std::make_tuple(18, 4), {DecimalField128(100000000, 4), DecimalField128(100010000, 4), DecimalField128(199990000, 4), DecimalField128(200000000, 4), DecimalField128(200010000, 4)})));
+    }
+
+    // decimal256
+    {
+        // int and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal64>>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}),
+            executeFunction(
+                func_name,
+                createColumn<Int32>({1, 1, 1, 1, 1}),
+                createColumn<Decimal256>(std::make_tuple(20, 4), {DecimalField256(Int256(100000000), 4), DecimalField256(Int256(100010000), 4), DecimalField256(Int256(199990000), 4), DecimalField256(Int256(200000000), 4), DecimalField256(Int256(200010000), 4)})));
+
+        // decimal and decimal
+        ASSERT_COLUMN_EQ(
+            createColumn<Nullable<Decimal128>>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}),
+            executeFunction(
+                func_name,
+                createColumn<Decimal256>(std::make_tuple(18, 4), {DecimalField256(Int256(10000), 4), DecimalField256(Int256(10000), 4), DecimalField256(Int256(10000), 4), DecimalField256(Int256(10000), 4), DecimalField256(Int256(10000), 4)}),
+                createColumn<Decimal256>(std::make_tuple(18, 4), {DecimalField256(Int256(100000000), 4), DecimalField256(Int256(100010000), 4), DecimalField256(Int256(199990000), 4), DecimalField256(Int256(200000000), 4), DecimalField256(Int256(200010000), 4)})));
+    }
+}
+CATCH
+
 TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimal)
 try
 {

diff --git a/tests/tidb-ci/fullstack-test-dt/issue_1425.test b/tests/tidb-ci/fullstack-test-dt/issue_1425.test
@@ -16,14 +16,21 @@ mysql> drop table if exists test.t;
 
 mysql> create table test.t (id int, value decimal(7,4), c1 int, c2 int);
 
-mysql> insert into test.t values(1,1.9286,54,28);
+mysql> insert into test.t values (1,1.9285,54,28), (1,1.9286,54,28);
 
 mysql> alter table test.t set tiflash replica 1;
 
 func> wait_table test t
 
+# note: ref to https://github.com/pingcap/tiflash/issues/1682,
+# The precision of tiflash results is different from that of tidb, which is a compatibility issue
 mysql> use test; set session tidb_isolation_read_engines='tiflash'; select * from t where value = 54/28;
 
 mysql> use test; set session tidb_isolation_read_engines='tiflash'; select * from t where value = c1/c2;
++------+--------+------+------+
+| id   | value  | c1   | c2   |
++------+--------+------+------+
+|    1 | 1.9286 |   54 |   28 |
++------+--------+------+------+
 
 mysql> drop table if exists test.t;