From f2f76548b90431dd27bf7c0352f2be2ee10178dc Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Thu, 8 Dec 2022 23:52:23 +0800 Subject: [PATCH 01/15] fix --- dbms/src/Functions/divide.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index 53c5de79448..8548fe4543f 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -62,7 +62,10 @@ struct TiDBDivideFloatingImpl template static Result apply(A a, B b) { - return static_cast(a) / b; + if constexpr (std::is_integral_v) + return (static_cast(a) + b / 2) / b; + else + return static_cast(a) / b; } template static Result apply(A a, B b, UInt8 & res_null) @@ -75,7 +78,7 @@ struct TiDBDivideFloatingImpl res_null = 1; return static_cast(0); } - return static_cast(a) / b; + return apply(a, b); } }; @@ -102,7 +105,7 @@ struct TiDBDivideFloatingImpl res_null = 1; return static_cast(0); } - return static_cast(a) / static_cast(b); + return apply(a, b); } }; @@ -332,4 +335,4 @@ void registerFunctionDivideIntegralOrZero(FunctionFactory & factory) factory.registerFunction(); } -} // namespace DB \ No newline at end of file +} // namespace DB From 2a529e5f46314c059eb8b15e3cda7efed9b3204b Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Mon, 12 Dec 2022 21:24:59 +0800 Subject: [PATCH 02/15] add test --- .../tests/gtest_arithmetic_functions.cpp | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp index 1d548a4c2d2..3b4ba00b736 100644 --- a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp +++ b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp @@ -103,6 +103,29 @@ class TestBinaryArithmeticFunctions : public DB::tests::FunctionTest } }; +TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRound) +try +{ + const String func_name = "tidbDivide"; + + // int and decimal + ASSERT_COLUMN_EQ( + createColumn>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}), + executeFunction( + func_name, + createColumn({1, 1, 1, 1, 1}), + createColumn(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>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}), + executeFunction( + func_name, + createColumn(std::make_tuple(18, 4), {DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4)}), + createColumn(std::make_tuple(18, 4), {DecimalField64(100000000, 4), DecimalField64(100010000, 4), DecimalField64(199990000, 4), DecimalField64(200000000, 4), DecimalField64(200010000, 4)}))); +} +CATCH + TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimal) try { From dbb471e54e8ad5e6984f58851bd0f952774e900a Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Tue, 13 Dec 2022 19:14:31 +0800 Subject: [PATCH 03/15] add more tests and fix bug --- dbms/src/Functions/divide.cpp | 3 +- .../tests/gtest_arithmetic_functions.cpp | 88 ++++++++++++++++--- 2 files changed, 76 insertions(+), 15 deletions(-) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index 8548fe4543f..37b57e97ffe 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -62,7 +62,8 @@ struct TiDBDivideFloatingImpl template static Result apply(A a, B b) { - if constexpr (std::is_integral_v) + // round for decimal floating number dividing + if constexpr (std::is_integral_v || std::is_same_v) return (static_cast(a) + b / 2) / b; else return static_cast(a) / b; diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp index 3b4ba00b736..5d658b570cc 100644 --- a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp +++ b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp @@ -108,21 +108,81 @@ try { const String func_name = "tidbDivide"; - // int and decimal - ASSERT_COLUMN_EQ( - createColumn>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}), - executeFunction( - func_name, - createColumn({1, 1, 1, 1, 1}), - createColumn(std::make_tuple(20, 4), {DecimalField64(100000000, 4), DecimalField64(100010000, 4), DecimalField64(199990000, 4), DecimalField64(200000000, 4), DecimalField64(200010000, 4)}))); + // decimal32 + { + // int and decimal + ASSERT_COLUMN_EQ( + createColumn>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}), + executeFunction( + func_name, + createColumn({1, 1, 1, 1, 1}), + createColumn(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>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}), + executeFunction( + func_name, + createColumn(std::make_tuple(18, 4), {DecimalField32(10000, 4), DecimalField32(10000, 4), DecimalField32(10000, 4), DecimalField32(10000, 4), DecimalField32(10000, 4)}), + createColumn(std::make_tuple(18, 4), {DecimalField32(100000000, 4), DecimalField32(100010000, 4), DecimalField32(199990000, 4), DecimalField32(200000000, 4), DecimalField32(200010000, 4)}))); + } - // decimal and decimal - ASSERT_COLUMN_EQ( - createColumn>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}), - executeFunction( - func_name, - createColumn(std::make_tuple(18, 4), {DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4)}), - createColumn(std::make_tuple(18, 4), {DecimalField64(100000000, 4), DecimalField64(100010000, 4), DecimalField64(199990000, 4), DecimalField64(200000000, 4), DecimalField64(200010000, 4)}))); + // decimal64 + { + // int and decimal + ASSERT_COLUMN_EQ( + createColumn>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}), + executeFunction( + func_name, + createColumn({1, 1, 1, 1, 1}), + createColumn(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>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}), + executeFunction( + func_name, + createColumn(std::make_tuple(18, 4), {DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4), DecimalField64(10000, 4)}), + createColumn(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>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}), + executeFunction( + func_name, + createColumn({1, 1, 1, 1, 1}), + createColumn(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>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}), + executeFunction( + func_name, + createColumn(std::make_tuple(18, 4), {DecimalField128(10000, 4), DecimalField128(10000, 4), DecimalField128(10000, 4), DecimalField128(10000, 4), DecimalField128(10000, 4)}), + createColumn(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>(std::make_tuple(18, 4), {DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(1, 4), DecimalField64(0, 4)}), + executeFunction( + func_name, + createColumn({1, 1, 1, 1, 1}), + createColumn(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>(std::make_tuple(26, 8), {DecimalField128(10000, 8), DecimalField128(9999, 8), DecimalField128(5000, 8), DecimalField128(5000, 8), DecimalField128(5000, 8)}), + executeFunction( + func_name, + createColumn(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(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 From 05fbb66599c940e4fa4fa2d946025861810a9c2d Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Tue, 7 Feb 2023 11:14:40 +0800 Subject: [PATCH 04/15] test stage --- dbms/src/Common/setThreadName.cpp | 12 ++++++- dbms/src/Functions/divide.cpp | 2 +- .../tests/gtest_arithmetic_functions.cpp | 33 +++++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/dbms/src/Common/setThreadName.cpp b/dbms/src/Common/setThreadName.cpp index 4d0aadd26ba..4b9739b8416 100644 --- a/dbms/src/Common/setThreadName.cpp +++ b/dbms/src/Common/setThreadName.cpp @@ -77,5 +77,15 @@ std::string getThreadName() std::string getThreadNameAndID() { - return getThreadName() + "_" + DB::toString(pthread_self()); + uint64_t thread_id; +#if defined(__APPLE__) + int err = pthread_threadid_np(pthread_self(), &thread_id); + if (err) + DB::throwFromErrno("Cannot get thread id with pthread_threadid_np()", DB::ErrorCodes::PTHREAD_ERROR, err); +#elif defined(__FreeBSD__) + thread_id = pthread_getthreadid_np(); +#else + thread_id = pthread_self(); +#endif + return getThreadName() + "_" + DB::toString(thread_id); } diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index 37b57e97ffe..b3f80aa0ed9 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -64,7 +64,7 @@ struct TiDBDivideFloatingImpl { // round for decimal floating number dividing if constexpr (std::is_integral_v || std::is_same_v) - return (static_cast(a) + b / 2) / b; + return (a < 0) != (b < 0) ? (a - (b / 2)) / b : (a + (b / 2)) / b; else return static_cast(a) / b; } diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp index 5d658b570cc..f45aaf6488e 100644 --- a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp +++ b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp @@ -19,7 +19,9 @@ #include #include #include +#include +#include #include #include #include @@ -103,6 +105,37 @@ class TestBinaryArithmeticFunctions : public DB::tests::FunctionTest } }; +TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRoundInternal) +try +{ + using TYPE = Int32; + auto apply = static_cast(&TiDBDivideFloatingImpl::apply); + + constexpr auto max = std::numeric_limits::max(); + // constexpr auto min = std::numeric_limits::min(); + // clang-format off + const std::vector> 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, max, 1}, {max-1, max, 1}, {max/2, max, 1}, {max/2-1, max, 0}, {0, max, 0} + }; + // clang-format on + + for (const auto & c : cases) + { + std::array res = {c[0], c[1], apply(c[0], c[1])}; + ASSERT_EQ(res, c); + } +} +CATCH + TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRound) try { From b1754223cf82434364ae0a3fafac78636d2eae2f Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Wed, 8 Feb 2023 15:20:31 +0800 Subject: [PATCH 05/15] basic impl --- dbms/src/Functions/divide.cpp | 21 +++++++++-- .../tests/gtest_arithmetic_functions.cpp | 35 ++++++++++++++----- 2 files changed, 44 insertions(+), 12 deletions(-) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index b3f80aa0ed9..6d25319ed9c 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -60,13 +60,28 @@ struct TiDBDivideFloatingImpl using ResultType = typename NumberTraits::ResultOfFloatingPointDivision::Type; template - static Result apply(A a, B b) + static Result apply(A x, B d) { // round for decimal floating number dividing if constexpr (std::is_integral_v || std::is_same_v) - return (a < 0) != (b < 0) ? (a - (b / 2)) / b : (a + (b / 2)) / b; + { + Result quotient = x / d; + Result mod = x % d; + Result half = (d / 2) + (d % 2); + + Result abs_m = mod < 0 ? -mod : mod; + Result abs_h = half < 0 ? -half : half; + if (abs_m >= abs_h) + { + if ((x < 0) == (d < 0)) // same_sign + quotient = quotient + 1; + else + quotient = quotient - 1; + } + return quotient; + } else - return static_cast(a) / b; + return static_cast(x) / d; } template static Result apply(A a, B b, UInt8 & res_null) diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp index f45aaf6488e..8816da68efd 100644 --- a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp +++ b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp @@ -105,14 +105,14 @@ class TestBinaryArithmeticFunctions : public DB::tests::FunctionTest } }; -TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRoundInternal) -try +template +void doTest() { - using TYPE = Int32; auto apply = static_cast(&TiDBDivideFloatingImpl::apply); - constexpr auto max = std::numeric_limits::max(); - // constexpr auto min = std::numeric_limits::min(); + constexpr TYPE max = std::numeric_limits::max(); + constexpr TYPE min = std::numeric_limits::min(); // note: Int256's min is not equal to -max-1 + // clang-format off const std::vector> cases = { {1, 2, 1}, {1, -2, -1}, {-1, 2, -1}, {-1, -2, 1}, @@ -124,16 +124,33 @@ try {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, max, 1}, {max-1, max, 1}, {max/2, max, 1}, {max/2-1, max, 0}, {0, max, 0} + // 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}, + + // 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 & c : cases) + for (const auto & expect : cases) { - std::array res = {c[0], c[1], apply(c[0], c[1])}; - ASSERT_EQ(res, c); + std::array actual = {expect[0], expect[1], apply(expect[0], expect[1])}; + ASSERT_EQ(expect, actual); } } + +TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRoundInternal) +try +{ + doTest(); + doTest(); + doTest(); + doTest(); +} CATCH TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRound) From c2068b4b9c4e03ef2c1121ec1d116c47111820d3 Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Wed, 8 Feb 2023 15:38:47 +0800 Subject: [PATCH 06/15] refine comment --- dbms/src/Functions/divide.cpp | 2 +- dbms/src/Functions/tests/gtest_arithmetic_functions.cpp | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index 6d25319ed9c..3e1fcbd0dab 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -73,7 +73,7 @@ struct TiDBDivideFloatingImpl Result abs_h = half < 0 ? -half : half; if (abs_m >= abs_h) { - if ((x < 0) == (d < 0)) // same_sign + if ((x < 0) == (d < 0)) // same_sign, i.e., quotient >= 0 quotient = quotient + 1; else quotient = quotient - 1; diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp index 8816da68efd..464689c39d8 100644 --- a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp +++ b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp @@ -111,7 +111,9 @@ void doTest() auto apply = static_cast(&TiDBDivideFloatingImpl::apply); constexpr TYPE max = std::numeric_limits::max(); - constexpr TYPE min = std::numeric_limits::min(); // note: Int256's min is not equal to -max-1 + constexpr TYPE min = std::numeric_limits::min(); + // 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 // clang-format off const std::vector> cases = { From 90df89bcd05db6a44073105b2d6a93296a12ffa1 Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Wed, 8 Feb 2023 16:51:05 +0800 Subject: [PATCH 07/15] tweaking --- dbms/src/Functions/divide.cpp | 3 ++- .../tests/gtest_arithmetic_functions.cpp | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index 3e1fcbd0dab..e0ddd7904a1 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -62,7 +62,8 @@ struct TiDBDivideFloatingImpl template static Result apply(A x, B d) { - // round for decimal floating number dividing + /// 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. if constexpr (std::is_integral_v || std::is_same_v) { Result quotient = x / d; diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp index 464689c39d8..8f281224d20 100644 --- a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp +++ b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp @@ -106,14 +106,14 @@ class TestBinaryArithmeticFunctions : public DB::tests::FunctionTest }; template -void doTest() +void doTiDBDivideDecimalRoundInternalTest() { auto apply = static_cast(&TiDBDivideFloatingImpl::apply); constexpr TYPE max = std::numeric_limits::max(); - constexpr TYPE min = std::numeric_limits::min(); // 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::min(); // clang-format off const std::vector> cases = { @@ -126,11 +126,13 @@ void doTest() {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 + // ±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, -1}, {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 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}, @@ -148,10 +150,10 @@ void doTest() TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRoundInternal) try { - doTest(); - doTest(); - doTest(); - doTest(); + doTiDBDivideDecimalRoundInternalTest(); + doTiDBDivideDecimalRoundInternalTest(); + doTiDBDivideDecimalRoundInternalTest(); + doTiDBDivideDecimalRoundInternalTest(); } CATCH From 1805d3d7fbc4edda70b052bfda86a92ebd039444 Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Wed, 8 Feb 2023 18:05:16 +0800 Subject: [PATCH 08/15] more comment --- dbms/src/Functions/divide.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index e0ddd7904a1..5c8699223bb 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -64,6 +64,7 @@ struct TiDBDivideFloatingImpl { /// 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 || std::is_same_v) { Result quotient = x / d; From 2369f9a521567731b0349fc1231341d1d81c91a5 Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Wed, 8 Feb 2023 18:40:24 +0800 Subject: [PATCH 09/15] add fullstack test --- tests/fullstack-test/expr/decimal_divide.test | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 tests/fullstack-test/expr/decimal_divide.test diff --git a/tests/fullstack-test/expr/decimal_divide.test b/tests/fullstack-test/expr/decimal_divide.test new file mode 100644 index 00000000000..5a43ec914fc --- /dev/null +++ b/tests/fullstack-test/expr/decimal_divide.test @@ -0,0 +1,137 @@ +# Copyright 2023 PingCAP, Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# decimal / decimal +mysql> drop table if exists test.t; +mysql> create table test.t(a decimal(4,0), b decimal(40, 20)); +mysql> alter table test.t set tiflash replica 1 +mysql> insert into test.t values (1, 10000), (1, 10001), (1, 20000), (1, 20001); +func> wait_table test t +mysql> set tidb_enforce_mpp=1; select a, b, a/b from test.t order by b; ++------+----------------------------+--------+ +| a | b | a/b | ++------+----------------------------+--------+ +| 1 | 10000.00000000000000000000 | 0.0001 | +| 1 | 10001.00000000000000000000 | 0.0001 | +| 1 | 20000.00000000000000000000 | 0.0001 | +| 1 | 20001.00000000000000000000 | 0.0000 | ++------+----------------------------+--------+ + +# int / decimal +mysql> drop table if exists test.t; +mysql> create table test.t(a int, b decimal(40, 20)); +mysql> alter table test.t set tiflash replica 1 +mysql> insert into test.t values (1, 10000), (1, 10001), (1, 20000), (1, 20001); +func> wait_table test t +mysql> set tidb_enforce_mpp=1; select a, b, a/b from test.t order by b; ++------+----------------------------+--------+ +| a | b | a/b | ++------+----------------------------+--------+ +| 1 | 10000.00000000000000000000 | 0.0001 | +| 1 | 10001.00000000000000000000 | 0.0001 | +| 1 | 20000.00000000000000000000 | 0.0001 | +| 1 | 20001.00000000000000000000 | 0.0000 | ++------+----------------------------+--------+ + +# decimal / int +mysql> drop table if exists test.t; +mysql> create table test.t(a int, b decimal(40, 20)); +mysql> alter table test.t set tiflash replica 1 +mysql> insert into test.t values (1, 10000), (1, 10001), (1, 20000), (1, 20001); +func> wait_table test t +mysql> set tidb_enforce_mpp=1; select a, b, a/b from test.t order by b; ++------+----------------------------+--------+ +| a | b | a/b | ++------+----------------------------+--------+ +| 1 | 10000.00000000000000000000 | 0.0001 | +| 1 | 10001.00000000000000000000 | 0.0001 | +| 1 | 20000.00000000000000000000 | 0.0001 | +| 1 | 20001.00000000000000000000 | 0.0000 | ++------+----------------------------+--------+ + +# int / int +mysql> drop table if exists test.t; +mysql> create table test.t(a int, b int); +mysql> alter table test.t set tiflash replica 1 +mysql> insert into test.t values (1, 10000), (1, 10001), (1, 20000), (1, 20001); +func> wait_table test t +mysql> set tidb_enforce_mpp=1; select a, b, a/b from test.t order by b; ++------+-------+--------+ +| a | b | a/b | ++------+-------+--------+ +| 1 | 10000 | 0.0001 | +| 1 | 10001 | 0.0001 | +| 1 | 20000 | 0.0001 | +| 1 | 20001 | 0.0000 | ++------+-------+--------+ + +mysql> drop table if exists test.t; +mysql> create table test.t(a decimal(10,0), b decimal(10,0)); +mysql> alter table test.t set tiflash replica 1 +mysql> insert into test.t values (2147483647, 1), (2147483647, 1073741823), (2147483647, 1073741824), (2147483647, 2147483646), (2147483647, 2147483647); +mysql> insert into test.t values (-2147483647, 1), (-2147483647, 1073741823), (-2147483647, 1073741824), (-2147483647, 2147483646), (-2147483647, 2147483647); +mysql> insert into test.t values (-2147483647, -1), (-2147483647, -1073741823), (-2147483647, -1073741824), (-2147483647, -2147483646), (-2147483647, -2147483647); +mysql> insert into test.t values (2147483647, -1), (2147483647, -1073741823), (2147483647, -1073741824), (2147483647, -2147483646), (2147483647, -2147483647); +func> wait_table test t +mysql> set tidb_enforce_mpp=1; select b, a, b/(a*10000) from test.t where a/b order by b; ++-------------+-------------+-------------+ +| b | a | b/(a*10000) | ++-------------+-------------+-------------+ +| -2147483647 | 2147483647 | -0.0001 | +| -2147483647 | -2147483647 | 0.0001 | +| -2147483646 | 2147483647 | -0.0001 | +| -2147483646 | -2147483647 | 0.0001 | +| -1073741824 | 2147483647 | -0.0001 | +| -1073741824 | -2147483647 | 0.0001 | +| -1073741823 | -2147483647 | 0.0000 | +| -1073741823 | 2147483647 | 0.0000 | +| -1 | 2147483647 | 0.0000 | +| -1 | -2147483647 | 0.0000 | +| 1 | -2147483647 | 0.0000 | +| 1 | 2147483647 | 0.0000 | +| 1073741823 | -2147483647 | 0.0000 | +| 1073741823 | 2147483647 | 0.0000 | +| 1073741824 | -2147483647 | -0.0001 | +| 1073741824 | 2147483647 | 0.0001 | +| 2147483646 | -2147483647 | -0.0001 | +| 2147483646 | 2147483647 | 0.0001 | +| 2147483647 | -2147483647 | -0.0001 | +| 2147483647 | 2147483647 | 0.0001 | ++-------------+-------------+-------------+ +mysql> delete from test.t; +mysql> insert into test.t values (2147483647, 9999999999), (9999999999, 2147483647), (1, 9999999999), (4999999999, 9999999999), (5000000000, 9999999999); +mysql> insert into test.t values (-2147483647, 9999999999), (-9999999999, 2147483647), (-1, 9999999999), (-4999999999, 9999999999), (-5000000000, 9999999999); +mysql> insert into test.t values (-2147483647, -9999999999), (-9999999999, -2147483647), (-1, -9999999999), (-4999999999, -9999999999), (-5000000000, -9999999999); +mysql> insert into test.t values (2147483647, -9999999999), (9999999999, -2147483647), (1, -9999999999), (4999999999, -9999999999), (5000000000, -9999999999); +mysql> set tidb_enforce_mpp=1; select b, a, b/(a*10000) from test.t where a/b order by b; ++-------------+-------------+-------------+ +| b | a | b/(a*10000) | ++-------------+-------------+-------------+ +| -9999999999 | 2147483647 | -0.0005 | +| -9999999999 | -4999999999 | 0.0002 | +| -9999999999 | 5000000000 | -0.0002 | +| -9999999999 | 4999999999 | -0.0002 | +| -9999999999 | -2147483647 | 0.0005 | +| -9999999999 | -5000000000 | 0.0002 | +| -2147483647 | -9999999999 | 0.0000 | +| -2147483647 | 9999999999 | 0.0000 | +| 2147483647 | 9999999999 | 0.0000 | +| 2147483647 | -9999999999 | 0.0000 | +| 9999999999 | -4999999999 | -0.0002 | +| 9999999999 | -2147483647 | -0.0005 | +| 9999999999 | -5000000000 | -0.0002 | +| 9999999999 | 2147483647 | 0.0005 | +| 9999999999 | 5000000000 | 0.0002 | +| 9999999999 | 4999999999 | 0.0002 | ++-------------+-------------+-------------+ \ No newline at end of file From 5b94bd027dedabbb7203a225b0a1f9e170d0cc6e Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Thu, 9 Feb 2023 14:06:09 +0800 Subject: [PATCH 10/15] add comments --- dbms/src/Functions/divide.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index 5c8699223bb..4fd1d38e46b 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -67,14 +67,20 @@ struct TiDBDivideFloatingImpl /// basically refer to https://stackoverflow.com/a/71634489 if constexpr (std::is_integral_v || std::is_same_v) { + /// 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, because 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 From eb005bc0d023538f3d7bab0d88709ed5858590ec Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Thu, 9 Feb 2023 14:08:48 +0800 Subject: [PATCH 11/15] EOL on EOF --- tests/fullstack-test/expr/decimal_divide.test | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fullstack-test/expr/decimal_divide.test b/tests/fullstack-test/expr/decimal_divide.test index 5a43ec914fc..98056ca556f 100644 --- a/tests/fullstack-test/expr/decimal_divide.test +++ b/tests/fullstack-test/expr/decimal_divide.test @@ -134,4 +134,4 @@ mysql> set tidb_enforce_mpp=1; select b, a, b/(a*10000) from test.t where a/b or | 9999999999 | 2147483647 | 0.0005 | | 9999999999 | 5000000000 | 0.0002 | | 9999999999 | 4999999999 | 0.0002 | -+-------------+-------------+-------------+ \ No newline at end of file ++-------------+-------------+-------------+ From 4ca7d3070afc7d9e21f7e252c6fb224216428602 Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Thu, 9 Feb 2023 14:10:14 +0800 Subject: [PATCH 12/15] tweaking --- dbms/src/Functions/divide.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Functions/divide.cpp b/dbms/src/Functions/divide.cpp index 4fd1d38e46b..132f6fae623 100644 --- a/dbms/src/Functions/divide.cpp +++ b/dbms/src/Functions/divide.cpp @@ -71,7 +71,7 @@ struct TiDBDivideFloatingImpl 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, because it may cause unexpected result. + /// 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. From 7686a93726733b93bbcf13c80fa8a0a7b25b8082 Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Thu, 9 Feb 2023 14:55:55 +0800 Subject: [PATCH 13/15] fix test --- dbms/src/Functions/tests/gtest_arithmetic_functions.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp index 8f281224d20..4ba24d677f3 100644 --- a/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp +++ b/dbms/src/Functions/tests/gtest_arithmetic_functions.cpp @@ -129,7 +129,7 @@ void doTiDBDivideDecimalRoundInternalTest() // ±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, -1}, {max/2+1, -max, -1}, {max-1, -max, -1}, {max, -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 From 021921499d80560ce7f6ad25396272c9d026c1dd Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Thu, 9 Feb 2023 15:40:47 +0800 Subject: [PATCH 14/15] disable issue_1425.test --- .../tidb-ci/fullstack-test-dt/issue_1425.test | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/tests/tidb-ci/fullstack-test-dt/issue_1425.test b/tests/tidb-ci/fullstack-test-dt/issue_1425.test index ca9106cc397..c951dc35a69 100644 --- a/tests/tidb-ci/fullstack-test-dt/issue_1425.test +++ b/tests/tidb-ci/fullstack-test-dt/issue_1425.test @@ -12,18 +12,18 @@ # See the License for the specific language governing permissions and # limitations under the License. -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> alter table test.t set tiflash replica 1; - -func> wait_table test t - -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; - -mysql> drop table if exists test.t; +# 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> alter table test.t set tiflash replica 1; +# +# func> wait_table test t +# +# 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; +# +# mysql> drop table if exists test.t; From f1a9064a81bbec081eff1e26a5e5b458c1476b2d Mon Sep 17 00:00:00 2001 From: Zhi Qi Date: Thu, 9 Feb 2023 15:57:29 +0800 Subject: [PATCH 15/15] refine test --- .../tidb-ci/fullstack-test-dt/issue_1425.test | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/tests/tidb-ci/fullstack-test-dt/issue_1425.test b/tests/tidb-ci/fullstack-test-dt/issue_1425.test index c951dc35a69..994e7d8abeb 100644 --- a/tests/tidb-ci/fullstack-test-dt/issue_1425.test +++ b/tests/tidb-ci/fullstack-test-dt/issue_1425.test @@ -12,18 +12,24 @@ # See the License for the specific language governing permissions and # limitations under the License. -# 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> alter table test.t set tiflash replica 1; -# -# func> wait_table test t -# -# 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; -# -# mysql> drop table if exists test.t; +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.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;