pitrou commented on a change in pull request #8542:
URL: https://github.com/apache/arrow/pull/8542#discussion_r521362458
##########
File path: cpp/src/arrow/util/basic_decimal.cc
##########
@@ -549,24 +603,27 @@ static DecimalStatus SingleDivide(const uint32_t*
dividend, int64_t dividend_len
return DecimalStatus::kSuccess;
}
-DecimalStatus BasicDecimal128::Divide(const BasicDecimal128& divisor,
- BasicDecimal128* result,
- BasicDecimal128* remainder) const {
+/// \brief Do a division where the divisor fits into a single 32 bit value.
+template <class DecimalClass>
+static inline DecimalStatus DecimalDivide(const DecimalClass& dividend,
+ const DecimalClass& divisor,
+ DecimalClass* result, DecimalClass*
remainder) {
+ constexpr int64_t kDecimalArrayLength = sizeof(DecimalClass) /
sizeof(uint32_t);
Review comment:
Use of `sizeof(DecimalClass)` here seems a bit fragile. Perhaps add/use
`DecimalClass::bit_width`?
##########
File path: cpp/src/arrow/util/decimal_test.cc
##########
@@ -1296,6 +1296,56 @@ TEST(Decimal256Test, Multiply) {
}
}
+TEST(Decimal256Test, Divide) {
+ ASSERT_EQ(Decimal256(33), Decimal256(100) / Decimal256(3));
+ ASSERT_EQ(Decimal256(66), Decimal256(200) / Decimal256(3));
+ ASSERT_EQ(Decimal256(66), Decimal256(20100) / Decimal256(301));
+ ASSERT_EQ(Decimal256(-66), Decimal256(-20100) / Decimal256(301));
+ ASSERT_EQ(Decimal256(-66), Decimal256(20100) / Decimal256(-301));
+ ASSERT_EQ(Decimal256(66), Decimal256(-20100) / Decimal256(-301));
+ ASSERT_EQ(Decimal256("-5192296858534827628530496329343552"),
+
Decimal256("-269599466671506397946670150910580797473777870509761363"
+ "24636208709184") /
+ Decimal256("5192296858534827628530496329874417"));
+ ASSERT_EQ(Decimal256("5192296858534827628530496329343552"),
+
Decimal256("-269599466671506397946670150910580797473777870509761363"
+ "24636208709184") /
+ Decimal256("-5192296858534827628530496329874417"));
+ ASSERT_EQ(Decimal256("5192296858534827628530496329343552"),
+
Decimal256("2695994666715063979466701509105807974737778705097613632"
+ "4636208709184") /
+ Decimal256("5192296858534827628530496329874417"));
+ ASSERT_EQ(Decimal256("-5192296858534827628530496329343552"),
+
Decimal256("2695994666715063979466701509105807974737778705097613632"
+ "4636208709184") /
+ Decimal256("-5192296858534827628530496329874417"));
+
+ // Test some random numbers.
+ for (auto x : GetRandomNumbers<Int32Type>(16)) {
+ for (auto y : GetRandomNumbers<Int32Type>(16)) {
+ if (y == 0) {
+ continue;
+ }
+
+ Decimal256 result = Decimal256(x) / Decimal256(y);
+ ASSERT_EQ(Decimal256(static_cast<int64_t>(x) / y), result)
+ << " x: " << x << " y: " << y;
+ }
+ }
+
+ // Test some edge cases
+ for (auto x : std::vector<int128_t>{-INT64_MAX, -INT32_MAX, 0, INT32_MAX,
INT64_MAX}) {
+ for (auto y : std::vector<int128_t>{-INT32_MAX, -32, -2, -1, 1, 2, 32,
INT32_MAX}) {
Review comment:
I would expect those additional values to also be part of `y`.
##########
File path: cpp/src/arrow/util/basic_decimal.cc
##########
@@ -549,24 +603,27 @@ static DecimalStatus SingleDivide(const uint32_t*
dividend, int64_t dividend_len
return DecimalStatus::kSuccess;
}
-DecimalStatus BasicDecimal128::Divide(const BasicDecimal128& divisor,
- BasicDecimal128* result,
- BasicDecimal128* remainder) const {
+/// \brief Do a division where the divisor fits into a single 32 bit value.
Review comment:
The comment here seems wrong (the divisor doesn't fit into a single
32bit value).
##########
File path: cpp/src/arrow/util/decimal_test.cc
##########
@@ -40,6 +40,9 @@ namespace arrow {
using internal::int128_t;
using internal::uint128_t;
+constexpr int128_t kInt128Max = (static_cast<int128_t>(INT64_MAX) << 64) - 1 +
+ 2 * (static_cast<int128_t>(INT64_MAX) + 1);
Review comment:
Why not `(static_cast<int128_t>(INT64_MAX) << 64) +
static_cast<int128_t>(UINT64_MAX)` ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
