Skip to content

MINOR: [C++] Reject out-of-range scale deltas in decimal Rescale #55159

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

Closed
Nayana-Naik73 wants to merge 1 commit into from
+47 −3
Closed

MINOR: [C++] Reject out-of-range scale deltas in decimal Rescale #55159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 7 additions & 3 deletions cpp/src/arrow/compute/kernels/scalar_cast_numeric.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,9 +675,13 @@ struct StringToDecimal {

if (ARROW_PREDICT_TRUE(r_parse.ok())) {
if (allow_truncate_) {
return (parsed_scale < out_scale_)
? parsed_out.IncreaseScaleBy(out_scale_ - parsed_scale)
: parsed_out.ReduceScaleBy(parsed_scale - out_scale_, false);
if (parsed_scale < out_scale_) {
return parsed_out.IncreaseScaleBy(out_scale_ - parsed_scale);
}
if (parsed_scale - out_scale_ > OutValue::kMaxScale) {
return {}; // Zero: every digit is truncated
}
return parsed_out.ReduceScaleBy(parsed_scale - out_scale_, false);
}

auto maybe_rescaled = parsed_out.Rescale(parsed_scale, out_scale_);
Expand Down
11 changes: 11 additions & 0 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 @@ -3123,6 +3123,17 @@ TEST(Cast, StringToDecimal) {
CheckCastFails(ArrayFromJSON(string_type, "[\"" + not_decimal + "\"]"), options);
}

// Parsed scale far beyond the type's maximum scale
auto tiny = ArrayFromJSON(string_type, R"(["1E-100000", "-1E-100000"])");
CheckCastFails(tiny, CastOptions::Safe(decimal_type));
CheckCast(ArrayFromJSON(string_type, R"(["0E-100000"])"),
ArrayFromJSON(decimal_type, R"(["0.00"])"));
auto truncate_options = CastOptions::Safe(decimal_type);
truncate_options.allow_decimal_truncate = true;
ASSERT_OK_AND_ASSIGN(auto truncated, Cast(tiny, truncate_options));
AssertArraysEqual(*ArrayFromJSON(decimal_type, R"(["0.00", "0.00"])"),
*truncated.make_array(), /*verbose=*/true);

#if !defined(_WIN32) || defined(NDEBUG)
// Test that casting is locale-independent
// French locale uses the comma as decimal point
Expand Down
6 changes: 6 additions & 0 deletions cpp/src/arrow/csv/converter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -734,6 +734,12 @@ TEST(DecimalConversion, Basics) {
AssertConversion<Decimal128Type, Decimal128>(
decimal128(23, 2), {"12,34.5\n", "36.37,-1e5\n"},
{{Dec128("12.00"), Dec128("36.37")}, {Dec128("34.50"), Dec128("-100000.00")}});

// Scale far beyond the maximum decimal128 scale
AssertConversion<Decimal128Type, Decimal128>(decimal128(10, 2), {"0E-100000\n"},
{{Dec128("0.00")}});
AssertConversionError(decimal128(10, 2), {"1E-41\n"}, {0});
AssertConversionError(decimal128(10, 2), {"1E-100000\n"}, {0});
}

TEST(DecimalConversion, Nulls) {
Expand Down
10 changes: 10 additions & 0 deletions cpp/src/arrow/util/basic_decimal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1063,6 +1063,16 @@ DecimalStatus DecimalRescale(const DecimalClass& value, int32_t original_scale,
const int32_t delta_scale = new_scale - original_scale;
const int32_t abs_delta_scale = std::abs(delta_scale);

// No multiplier this large fits in the type (nor in the powers-of-ten table),
// so only zero can be rescaled without data loss.
if (ARROW_PREDICT_FALSE(abs_delta_scale > DecimalClass::kMaxScale)) {
if (value == 0) {
*out = value;
return DecimalStatus::kSuccess;
}
return DecimalStatus::kRescaleDataLoss;
}

DecimalClass multiplier = DecimalClass::GetScaleMultiplier(abs_delta_scale);

const bool rescale_would_cause_data_loss =
Expand Down
13 changes: 13 additions & 0 deletions cpp/src/arrow/util/decimal_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1568,6 +1568,13 @@ TYPED_TEST(TestBasicDecimalFunctionality, Rescale) {
ASSERT_OK_AND_EQ(TypeParam(500000), TypeParam(5).Rescale(1, 6));
ASSERT_RAISES(Invalid, TypeParam(555555).Rescale(6, 1));

// Scale deltas larger than kMaxScale
constexpr int32_t kMaxScale = TypeParam::kMaxScale;
ASSERT_RAISES(Invalid, TypeParam(1).Rescale(kMaxScale + 1, 0));
ASSERT_RAISES(Invalid, TypeParam(1).Rescale(0, kMaxScale + 1));
ASSERT_RAISES(Invalid, TypeParam(-1).Rescale(100000, 2));
ASSERT_OK_AND_EQ(TypeParam(0), TypeParam(0).Rescale(100000, 2));

using OrigScaleType =
std::conditional_t<std::is_same_v<TypeParam, Decimal32>, Int8Type, Int16Type>;
using ValueType =
Expand Down Expand Up @@ -2369,6 +2376,12 @@ TEST(Decimal256Test, Rescale) {
ASSERT_OK_AND_EQ(Decimal256(500000), Decimal256(5).Rescale(1, 6));
ASSERT_RAISES(Invalid, Decimal256(555555).Rescale(6, 1));

// Scale deltas larger than kMaxScale
ASSERT_RAISES(Invalid, Decimal256(1).Rescale(Decimal256::kMaxScale + 1, 0));
ASSERT_RAISES(Invalid, Decimal256(1).Rescale(0, Decimal256::kMaxScale + 1));
ASSERT_RAISES(Invalid, Decimal256(-1).Rescale(100000, 2));
ASSERT_OK_AND_EQ(Decimal256(0), Decimal256(0).Rescale(100000, 2));

// Test some random numbers.
for (auto original_scale : GetRandomNumbers<Int16Type>(16)) {
for (auto value : GetRandomNumbers<Int32Type>(16)) {
Expand Down
Loading