Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix the issue that decimal divide not round. #6471

Merged
merged 32 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f2f7654
fix
LittleFall Dec 8, 2022
2a529e5
add test
LittleFall Dec 12, 2022
dbb471e
add more tests and fix bug
LittleFall Dec 13, 2022
2c7961a
Merge branch 'master' into fix/decimal-divide-round
LittleFall Dec 13, 2022
87cc360
Merge branch 'master' into fix/decimal-divide-round
LittleFall Jan 13, 2023
496d3ad
Merge branch 'master' into fix/decimal-divide-round
LittleFall Jan 17, 2023
65056ce
Merge remote-tracking branch 'origin/master' into fix/decimal-divide-…
LittleFall Jan 18, 2023
609b152
Merge branch 'master' into fix/decimal-divide-round
LittleFall Feb 7, 2023
05fbb66
test stage
LittleFall Feb 7, 2023
ff58c5c
Merge remote-tracking branch 'origin/fix/decimal-divide-round' into f…
LittleFall Feb 7, 2023
b175422
basic impl
LittleFall Feb 8, 2023
c2068b4
refine comment
LittleFall Feb 8, 2023
90df89b
tweaking
LittleFall Feb 8, 2023
65a8533
Merge branch 'master' into fix/decimal-divide-round
wuhuizuo Feb 8, 2023
1805d3d
more comment
LittleFall Feb 8, 2023
6eecf77
Merge remote-tracking branch 'origin/fix/decimal-divide-round' into f…
LittleFall Feb 8, 2023
2369f9a
add fullstack test
LittleFall Feb 8, 2023
94f621f
Merge branch 'master' into fix/decimal-divide-round
LittleFall Feb 8, 2023
d78ad7b
Merge branch 'master' into fix/decimal-divide-round
bestwoody Feb 8, 2023
f97053f
Merge branch 'master' into fix/decimal-divide-round
bestwoody Feb 8, 2023
5b94bd0
add comments
LittleFall Feb 9, 2023
ad26758
Merge branch 'master' into fix/decimal-divide-round
LittleFall Feb 9, 2023
655bc16
Merge remote-tracking branch 'origin/fix/decimal-divide-round' into f…
LittleFall Feb 9, 2023
eb005bc
EOL on EOF
LittleFall Feb 9, 2023
4ca7d30
tweaking
LittleFall Feb 9, 2023
2f460ec
Merge branch 'master' into fix/decimal-divide-round
ti-chi-bot Feb 9, 2023
7686a93
fix test
LittleFall Feb 9, 2023
04012f1
Merge remote-tracking branch 'origin/fix/decimal-divide-round' into f…
LittleFall Feb 9, 2023
62db0b3
Merge branch 'master' into fix/decimal-divide-round
ti-chi-bot Feb 9, 2023
0219214
disable issue_1425.test
LittleFall Feb 9, 2023
153ea48
Merge remote-tracking branch 'origin/fix/decimal-divide-round' into f…
LittleFall Feb 9, 2023
f1a9064
refine test
LittleFall Feb 9, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions dbms/src/Functions/divide.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,30 @@ 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>)
{
Result quotient = x / d;
Result mod = x % d;
Result half = (d / 2) + (d % 2);

Result abs_m = mod < 0 ? -mod : mod;
LittleFall marked this conversation as resolved.
Show resolved Hide resolved
Result abs_h = half < 0 ? -half : half;
if (abs_m >= abs_h)
{
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)
Expand All @@ -75,7 +96,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);
}
};

Expand All @@ -102,7 +123,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);
}
};

Expand Down Expand Up @@ -332,4 +353,4 @@ void registerFunctionDivideIntegralOrZero(FunctionFactory & factory)
factory.registerFunction<FunctionDivideIntegralOrZero>();
}

} // namespace DB
} // namespace DB
137 changes: 137 additions & 0 deletions dbms/src/Functions/tests/gtest_arithmetic_functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -103,6 +105,141 @@ 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, -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, 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>();
}
CATCH

TEST_F(TestBinaryArithmeticFunctions, TiDBDivideDecimalRound)
LittleFall marked this conversation as resolved.
Show resolved Hide resolved
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}),
LittleFall marked this conversation as resolved.
Show resolved Hide resolved
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
{
Expand Down
137 changes: 137 additions & 0 deletions tests/fullstack-test/expr/decimal_divide.test
Original file line number Diff line number Diff line change
@@ -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 |
+-------------+-------------+-------------+