Skip to content

GH-35576: [C++] Make Decimal{128,256}::FromReal more accurate #35997

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.

Sign up for GitHub

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

Merged
pitrou merged 4 commits into from
Jun 14, 2023
Merged

GH-35576: [C++] Make Decimal{128,256}::FromReal more accurate #35997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2c78a97
GH-35576: [C++] Make Decimal{128,256}::FromReal more accurate
pitrou Jun 5, 2023
daef12b
Fix out of bounds read
pitrou Jun 8, 2023
747dfa6
Compatibility with older Pythons
pitrou Jun 8, 2023
87be93a
Address review comments and add a test
pitrou Jun 14, 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
4 changes: 2 additions & 2 deletions cpp/src/arrow/compute/kernels/scalar_cast_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,12 +1006,12 @@ TEST(Cast, FloatingToDecimal) {

CheckCast(ArrayFromJSON(float32(), "[1.8446746e+15, -1.8446746e+15]"),
ArrayFromJSON(decimal_type(20, 4),
R"(["1844674627273280.7168", "-1844674627273280.7168"])"));
R"(["1844674629206016.0000", "-1844674629206016.0000"])"));

CheckCast(
ArrayFromJSON(float64(), "[1.8446744073709556e+15, -1.8446744073709556e+15]"),
ArrayFromJSON(decimal_type(20, 4),
R"(["1844674407370955.5712", "-1844674407370955.5712"])"));
R"(["1844674407370955.5000", "-1844674407370955.5000"])"));

// Edge cases are tested for Decimal128::FromReal() and Decimal256::FromReal
}
Expand Down
408 changes: 53 additions & 355 deletions cpp/src/arrow/util/basic_decimal.cc
View file
Open in desktop

Large diffs are not rendered by default.

96 changes: 55 additions & 41 deletions cpp/src/arrow/util/basic_decimal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,16 @@ class ARROW_EXPORT GenericBasicDecimal {

#if ARROW_LITTLE_ENDIAN
static constexpr int kHighWordIndex = NWORDS - 1;
static constexpr int kLowWordIndex = 0;
#else
static constexpr int kHighWordIndex = 0;
static constexpr int kLowWordIndex = NWORDS - 1;
#endif

public:
static constexpr int kBitWidth = BIT_WIDTH;
static constexpr int kByteWidth = kBitWidth / 8;
static constexpr int kNumWords = NWORDS;

// A constructor tag to introduce a little-endian encoded array
static constexpr LittleEndianArrayTag LittleEndianArray{};
Expand All @@ -64,8 +67,7 @@ class ARROW_EXPORT GenericBasicDecimal {
/// \brief Create a decimal from the two's complement representation.
///
/// Input array is assumed to be in native endianness.
constexpr GenericBasicDecimal(
const WordArray& array) noexcept // NOLINT(runtime/explicit)
explicit constexpr GenericBasicDecimal(const WordArray& array) noexcept
: array_(array) {}

/// \brief Create a decimal from the two's complement representation.
Expand All @@ -74,6 +76,13 @@ class ARROW_EXPORT GenericBasicDecimal {
GenericBasicDecimal(LittleEndianArrayTag, const WordArray& array) noexcept
: GenericBasicDecimal(bit_util::little_endian::ToNative(array)) {}

/// \brief Create a decimal from any integer not wider than 64 bits.
template <typename T,
typename = typename std::enable_if<
std::is_integral<T>::value && (sizeof(T) <= sizeof(uint64_t)), T>::type>
constexpr GenericBasicDecimal(T value) noexcept // NOLINT(runtime/explicit)
: array_(WordsFromLowBits(value)) {}

/// \brief Create a decimal from an array of bytes.
///
/// Bytes are assumed to be in native-endian byte order.
Expand Down Expand Up @@ -124,8 +133,37 @@ class ARROW_EXPORT GenericBasicDecimal {

bool IsNegative() const { return static_cast<int64_t>(array_[kHighWordIndex]) < 0; }

explicit operator bool() const { return array_ != WordArray{}; }

friend bool operator==(const GenericBasicDecimal& left,
const GenericBasicDecimal& right) {
return left.array_ == right.array_;
}

friend bool operator!=(const GenericBasicDecimal& left,
const GenericBasicDecimal& right) {
return left.array_ != right.array_;
}

protected:
WordArray array_;

template <typename T>
static constexpr uint64_t SignExtend(T low_bits) noexcept {
return low_bits >= T{} ? uint64_t{0} : ~uint64_t{0};
}

template <typename T>
static constexpr WordArray WordsFromLowBits(T low_bits) {
WordArray words{};
if (low_bits < T{}) {
for (auto& word : words) {
word = ~uint64_t{0};
}
}
words[kLowWordIndex] = static_cast<uint64_t>(low_bits);
return words;
}
};

/// Represents a signed 128-bit integer in two's complement.
Expand All @@ -150,14 +188,6 @@ class ARROW_EXPORT BasicDecimal128 : public GenericBasicDecimal<BasicDecimal128,
: BasicDecimal128(WordArray{static_cast<uint64_t>(high), low}) {}
#endif

/// \brief Convert any integer value into a BasicDecimal128.
template <typename T,
typename = typename std::enable_if<
std::is_integral<T>::value && (sizeof(T) <= sizeof(uint64_t)), T>::type>
constexpr BasicDecimal128(T value) noexcept // NOLINT(runtime/explicit)
: BasicDecimal128(value >= T{0} ? 0 : -1, static_cast<uint64_t>(value)) { // NOLINT
}

/// \brief Negate the current value (in-place)
BasicDecimal128& Negate();

Expand Down Expand Up @@ -208,7 +238,9 @@ class ARROW_EXPORT BasicDecimal128 : public GenericBasicDecimal<BasicDecimal128,
return res;
}

/// \brief Shift right by the given number of bits. Negative values will
/// \brief Shift right by the given number of bits.
///
/// Negative values will sign-extend.
BasicDecimal128& operator>>=(uint32_t bits);

BasicDecimal128 operator>>(uint32_t bits) const {
Expand Down Expand Up @@ -284,8 +316,6 @@ class ARROW_EXPORT BasicDecimal128 : public GenericBasicDecimal<BasicDecimal128,
}
};

ARROW_EXPORT bool operator==(const BasicDecimal128& left, const BasicDecimal128& right);
ARROW_EXPORT bool operator!=(const BasicDecimal128& left, const BasicDecimal128& right);
ARROW_EXPORT bool operator<(const BasicDecimal128& left, const BasicDecimal128& right);
ARROW_EXPORT bool operator<=(const BasicDecimal128& left, const BasicDecimal128& right);
ARROW_EXPORT bool operator>(const BasicDecimal128& left, const BasicDecimal128& right);
Expand All @@ -305,14 +335,6 @@ ARROW_EXPORT BasicDecimal128 operator%(const BasicDecimal128& left,
const BasicDecimal128& right);

class ARROW_EXPORT BasicDecimal256 : public GenericBasicDecimal<BasicDecimal256, 256> {
private:
// Due to a bug in clang, we have to declare the extend method prior to its
// usage.
template <typename T>
static constexpr uint64_t extend(T low_bits) noexcept {
return low_bits >= T() ? uint64_t{0} : ~uint64_t{0};
}

public:
using GenericBasicDecimal::GenericBasicDecimal;

Expand All @@ -321,19 +343,10 @@ class ARROW_EXPORT BasicDecimal256 : public GenericBasicDecimal<BasicDecimal256,

constexpr BasicDecimal256() noexcept : GenericBasicDecimal() {}

/// \brief Convert any integer value into a BasicDecimal256.
template <typename T,
typename = typename std::enable_if<
std::is_integral<T>::value && (sizeof(T) <= sizeof(uint64_t)), T>::type>
constexpr BasicDecimal256(T value) noexcept // NOLINT(runtime/explicit)
: BasicDecimal256(bit_util::little_endian::ToNative<uint64_t, 4>(
{static_cast<uint64_t>(value), extend(value), extend(value),
extend(value)})) {}

explicit BasicDecimal256(const BasicDecimal128& value) noexcept
: BasicDecimal256(bit_util::little_endian::ToNative<uint64_t, 4>(
{value.low_bits(), static_cast<uint64_t>(value.high_bits()),
extend(value.high_bits()), extend(value.high_bits())})) {}
SignExtend(value.high_bits()), SignExtend(value.high_bits())})) {}

/// \brief Negate the current value (in-place)
BasicDecimal256& Negate();
Expand Down Expand Up @@ -403,6 +416,17 @@ class ARROW_EXPORT BasicDecimal256 : public GenericBasicDecimal<BasicDecimal256,
return res;
}

/// \brief Shift right by the given number of bits.
///
/// Negative values will sign-extend.
BasicDecimal256& operator>>=(uint32_t bits);

BasicDecimal256 operator>>(uint32_t bits) const {
auto res = *this;
res >>= bits;
return res;
}

/// \brief In-place division.
BasicDecimal256& operator/=(const BasicDecimal256& right);

Expand Down Expand Up @@ -435,16 +459,6 @@ class ARROW_EXPORT BasicDecimal256 : public GenericBasicDecimal<BasicDecimal256,
}
};

ARROW_EXPORT inline bool operator==(const BasicDecimal256& left,
const BasicDecimal256& right) {
return left.native_endian_array() == right.native_endian_array();
}

ARROW_EXPORT inline bool operator!=(const BasicDecimal256& left,
const BasicDecimal256& right) {
return left.native_endian_array() != right.native_endian_array();
}

ARROW_EXPORT bool operator<(const BasicDecimal256& left, const BasicDecimal256& right);

ARROW_EXPORT inline bool operator<=(const BasicDecimal256& left,
Expand Down
Loading