Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions src/iceberg/test/decimal_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <array>
#include <bit>
#include <cstdint>
#include <limits>

#include <gmock/gmock.h>
#include <gtest/gtest.h>
Expand Down Expand Up @@ -714,6 +715,16 @@ TEST(DecimalTest, Rescale) {
ASSERT_EQ(Decimal(500000), Decimal(5).Rescale(1, 6).value());

ASSERT_THAT(Decimal(5555555).Rescale(6, 1), IsError(ErrorKind::kInvalid));

// Scale deltas beyond kMaxScale must fail cleanly instead of indexing past
// the powers-of-ten table (previously DCHECK-only, a no-op under NDEBUG).
ASSERT_THAT(Decimal(1).Rescale(0, 100), IsError(ErrorKind::kInvalid));
ASSERT_THAT(Decimal(1).Rescale(100, 0), IsError(ErrorKind::kInvalid));
ASSERT_THAT(Decimal(1).Rescale(std::numeric_limits<int32_t>::min(),
std::numeric_limits<int32_t>::max()),
IsError(ErrorKind::kInvalid));
// Zero is exact at every scale.
ASSERT_EQ(Decimal(0), Decimal(0).Rescale(0, 100).value());
}

TEST(DecimalTest, Compare) {
Expand Down Expand Up @@ -768,6 +779,19 @@ TEST(DecimalTest, Compare) {
std::partial_ordering::less);
ASSERT_EQ(Decimal::Compare(Decimal("-123456789"), Decimal("0"), 2, 3),
std::partial_ordering::less);

// scale deltas beyond kMaxScale must not read past the powers-of-ten table
// (previously DCHECK-only, a no-op under NDEBUG); the lower-scale operand
// dominates because aligning it always overflows.
ASSERT_EQ(Decimal::Compare(Decimal(1), Decimal(5), 100, 0),
std::partial_ordering::less);
ASSERT_EQ(Decimal::Compare(Decimal(5), Decimal(1), 0, 100),
std::partial_ordering::greater);
ASSERT_EQ(Decimal::Compare(Decimal(-1), Decimal(-5), 100, 0),
std::partial_ordering::greater);
ASSERT_EQ(Decimal::Compare(Decimal(1), Decimal(1), std::numeric_limits<int32_t>::max(),
std::numeric_limits<int32_t>::min()),
std::partial_ordering::less);
}

} // namespace iceberg
10 changes: 10 additions & 0 deletions src/iceberg/test/type_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -519,6 +519,16 @@ TEST(TypeTest, Decimal) {
ASSERT_EQ(10, decimal.precision());
ASSERT_EQ(-10, decimal.scale());
}
{
iceberg::DecimalType decimal(38, 100);
ASSERT_EQ(38, decimal.precision());
ASSERT_EQ(100, decimal.scale());
}
{
iceberg::DecimalType decimal(38, -100);
ASSERT_EQ(38, decimal.precision());
ASSERT_EQ(-100, decimal.scale());
}
ASSERT_THAT([]() { iceberg::DecimalType decimal(-1, 10); },
::testing::ThrowsMessage<iceberg::IcebergError>(
::testing::HasSubstr("precision must be in [0, 38], was -1")));
Expand Down
40 changes: 34 additions & 6 deletions src/iceberg/util/decimal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -552,12 +552,26 @@ Result<Decimal> Decimal::Rescale(int32_t orig_scale, int32_t new_scale) const {
return *this;
}

const int32_t delta_scale = new_scale - orig_scale;
// Compute the delta in 64 bits so unvalidated scales cannot cause signed
// overflow, and range-check it in release builds: kDecimal128PowersOfTen
// has only kMaxScale + 1 entries, so a DCHECK alone would be an
// out-of-bounds read under NDEBUG.
const int64_t delta_scale64 =
static_cast<int64_t>(new_scale) - static_cast<int64_t>(orig_scale);
if (delta_scale64 > kMaxScale || delta_scale64 < -kMaxScale) [[unlikely]] {
if (data_ == 0) {
// Zero is exact at every scale.
return *this;
}
// Shifting a nonzero 128-bit decimal by more than kMaxScale digits either
// overflows (scaling up) or truncates every digit (scaling down).
return Invalid("Rescale {} from {} to {} would cause data loss", ToIntegerString(),
orig_scale, new_scale);
}
const auto delta_scale = static_cast<int32_t>(delta_scale64);
const int32_t abs_delta_scale = std::abs(delta_scale);
Decimal out;

ICEBERG_DCHECK(abs_delta_scale <= kMaxScale, "");

auto& multiplier = kDecimal128PowersOfTen[abs_delta_scale];

if (RescaleWouldCauseDataLoss(*this, delta_scale, multiplier, &out)) [[unlikely]] {
Expand Down Expand Up @@ -590,11 +604,25 @@ std::partial_ordering Decimal::Compare(const Decimal& lhs, const Decimal& rhs,
// Both are negative
bool negative = lhs.data_ < 0 && rhs.data_ < 0;

const int32_t delta_scale = lhs_scale - rhs_scale;
// Compute the delta in 64 bits so unvalidated scales cannot cause signed
// overflow, and range-check it in release builds: kDecimal128PowersOfTen
// has only kMaxScale + 1 entries, so a DCHECK alone would be an
// out-of-bounds read under NDEBUG.
const int64_t delta_scale64 =
static_cast<int64_t>(lhs_scale) - static_cast<int64_t>(rhs_scale);
if (delta_scale64 > kMaxScale || delta_scale64 < -kMaxScale) [[unlikely]] {
// Aligning the lower-scale operand would multiply a nonzero 128-bit value
// by more than 10^kMaxScale, which always overflows. That is exactly the
// data-loss case below, so return the same sign-based ordering. Both
// operands are nonzero here (handled above).
if (delta_scale64 < 0) {
return negative ? std::partial_ordering::less : std::partial_ordering::greater;
}
return negative ? std::partial_ordering::greater : std::partial_ordering::less;
}
const auto delta_scale = static_cast<int32_t>(delta_scale64);
const int32_t abs_delta_scale = std::abs(delta_scale);

ICEBERG_DCHECK(abs_delta_scale <= kMaxScale, "");

const auto& multiplier = kDecimal128PowersOfTen[abs_delta_scale];

Decimal adjusted_lhs;
Expand Down
Loading