You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by we...@apache.org on 2017/11/09 01:51:01 UTC
[arrow] branch master updated: ARROW-1709: [C++] Decimal.ToString
is incorrect for negative scale
This is an automated email from the ASF dual-hosted git repository.
wesm pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/master by this push:
new dffa486 ARROW-1709: [C++] Decimal.ToString is incorrect for negative scale
dffa486 is described below
commit dffa486c86d1d09c16e4c52ad0ff78bbee22c4e1
Author: Phillip Cloud <cp...@gmail.com>
AuthorDate: Wed Nov 8 20:50:51 2017 -0500
ARROW-1709: [C++] Decimal.ToString is incorrect for negative scale
This is on top of ARROW-1716. Will rebase when that's merged.
Author: Phillip Cloud <cp...@gmail.com>
Closes #1292 from cpcloud/ARROW-1709 and squashes the following commits:
13ef9a32 [Phillip Cloud] Fix formatting
e87e88f0 [Phillip Cloud] Cleanup
3431b667 [Phillip Cloud] Skip leading negative sign in dcheck
9874d84d [Phillip Cloud] Use lambda
3a5e3f6f [Phillip Cloud] DCHECK and format
2dde6d2d [Phillip Cloud] ARROW-1709: [C++] Decimal.ToString is incorrect for negative scale
---
cpp/src/arrow/util/decimal-test.cc | 75 ++++++++++++++++++++++++++
cpp/src/arrow/util/decimal.cc | 104 ++++++++++++++++++++++++++++++-------
2 files changed, 159 insertions(+), 20 deletions(-)
diff --git a/cpp/src/arrow/util/decimal-test.cc b/cpp/src/arrow/util/decimal-test.cc
index b0271ff..0d0c08c 100644
--- a/cpp/src/arrow/util/decimal-test.cc
+++ b/cpp/src/arrow/util/decimal-test.cc
@@ -18,6 +18,7 @@
#include <cstdint>
#include <string>
+#include <tuple>
#include <gtest/gtest.h>
@@ -291,4 +292,78 @@ TEST(Decimal128Test, PrintMinValue) {
ASSERT_EQ(string_value, printed_value);
}
+class Decimal128PrintingTest
+ : public ::testing::TestWithParam<std::tuple<int32_t, int32_t, std::string>> {};
+
+TEST_P(Decimal128PrintingTest, Print) {
+ int32_t test_value;
+ int32_t scale;
+ std::string expected_string;
+ std::tie(test_value, scale, expected_string) = GetParam();
+ const Decimal128 value(test_value);
+ const std::string printed_value = value.ToString(scale);
+ ASSERT_EQ(expected_string, printed_value);
+}
+
+INSTANTIATE_TEST_CASE_P(Decimal128PrintingTest, Decimal128PrintingTest,
+ ::testing::Values(std::make_tuple(123, 1, "12.3"),
+ std::make_tuple(123, 5, "0.00123"),
+ std::make_tuple(123, 10, "1.23E-8"),
+ std::make_tuple(123, -1, "1.23E+3"),
+ std::make_tuple(-123, -1, "-1.23E+3"),
+ std::make_tuple(123, -3, "1.23E+5"),
+ std::make_tuple(-123, -3, "-1.23E+5"),
+ std::make_tuple(12345, -3, "1.2345E+7")));
+
+class Decimal128ParsingTest
+ : public ::testing::TestWithParam<std::tuple<std::string, uint64_t, int32_t>> {};
+
+TEST_P(Decimal128ParsingTest, Parse) {
+ std::string test_string;
+ uint64_t expected_low_bits;
+ int32_t expected_scale;
+ std::tie(test_string, expected_low_bits, expected_scale) = GetParam();
+ Decimal128 value;
+ int32_t scale;
+ ASSERT_OK(Decimal128::FromString(test_string, &value, NULLPTR, &scale));
+ ASSERT_EQ(value.low_bits(), expected_low_bits);
+ ASSERT_EQ(expected_scale, scale);
+}
+
+INSTANTIATE_TEST_CASE_P(Decimal128ParsingTest, Decimal128ParsingTest,
+ ::testing::Values(std::make_tuple("12.3", 123ULL, 1),
+ std::make_tuple("0.00123", 123ULL, 5),
+ std::make_tuple("1.23E-8", 123ULL, 10),
+ std::make_tuple("-1.23E-8", -123LL, 10),
+ std::make_tuple("1.23E+3", 123ULL, -1),
+ std::make_tuple("-1.23E+3", -123LL, -1),
+ std::make_tuple("1.23E+5", 123ULL, -3),
+ std::make_tuple("1.2345E+7", 12345ULL, -3),
+ std::make_tuple("1.23e-8", 123ULL, 10),
+ std::make_tuple("-1.23e-8", -123LL, 10),
+ std::make_tuple("1.23e+3", 123ULL, -1),
+ std::make_tuple("-1.23e+3", -123LL, -1),
+ std::make_tuple("1.23e+5", 123ULL, -3),
+ std::make_tuple("1.2345e+7", 12345ULL, -3)));
+
+class Decimal128ParsingTestInvalid : public ::testing::TestWithParam<std::string> {};
+
+TEST_P(Decimal128ParsingTestInvalid, Parse) {
+ std::string test_string = GetParam();
+ Decimal128 value;
+ ASSERT_RAISES(Invalid, Decimal128::FromString(test_string, &value));
+}
+
+INSTANTIATE_TEST_CASE_P(Decimal128ParsingTestInvalid, Decimal128ParsingTestInvalid,
+ ::testing::Values("0.00123D/3", "1.23eA8", "1.23E+3A",
+ "-1.23E--5", "1.2345E+++07"));
+
+TEST(Decimal128ParseTest, WithExponentAndNullptrScale) {
+ Decimal128 value;
+ ASSERT_OK(Decimal128::FromString("1.23E-8", &value));
+
+ const Decimal128 expected_value(123);
+ ASSERT_EQ(expected_value, value);
+}
+
} // namespace arrow
diff --git a/cpp/src/arrow/util/decimal.cc b/cpp/src/arrow/util/decimal.cc
index cc18025..447cae5 100644
--- a/cpp/src/arrow/util/decimal.cc
+++ b/cpp/src/arrow/util/decimal.cc
@@ -105,6 +105,22 @@ Decimal128::operator int64_t() const {
return static_cast<int64_t>(low_bits_);
}
+static std::string ToStringNegativeScale(const std::string& str,
+ int32_t adjusted_exponent, bool is_negative) {
+ std::stringstream buf;
+
+ size_t offset = 0;
+ buf << str[offset++];
+
+ if (is_negative) {
+ buf << str[offset++];
+ }
+
+ buf << '.' << str.substr(offset, std::string::npos) << 'E' << std::showpos
+ << adjusted_exponent;
+ return buf.str();
+}
+
std::string Decimal128::ToString(int32_t scale) const {
const std::string str(ToIntegerString());
@@ -112,9 +128,18 @@ std::string Decimal128::ToString(int32_t scale) const {
return str;
}
- if (*this < 0) {
- const auto len = static_cast<int32_t>(str.size());
+ const bool is_negative = *this < 0;
+ const auto len = static_cast<int32_t>(str.size());
+ const auto is_negative_offset = static_cast<int32_t>(is_negative);
+ const int32_t adjusted_exponent = -scale + (len - 1 - is_negative_offset);
+
+ /// Note that the -6 is taken from the Java BigDecimal documentation.
+ if (scale < 0 || adjusted_exponent < -6) {
+ return ToStringNegativeScale(str, adjusted_exponent, is_negative);
+ }
+
+ if (is_negative) {
if (len - 1 > scale) {
const auto n = static_cast<size_t>(len - scale);
return str.substr(0, n) + "." + str.substr(n, static_cast<size_t>(scale));
@@ -128,8 +153,6 @@ std::string Decimal128::ToString(int32_t scale) const {
return result + str.substr(1, std::string::npos);
}
- const auto len = static_cast<int32_t>(str.size());
-
if (len > scale) {
const auto n = static_cast<size_t>(len - scale);
return str.substr(0, n) + "." + str.substr(n, static_cast<size_t>(scale));
@@ -164,10 +187,12 @@ static constexpr int64_t kPowersOfTen[kInt64DecimalDigits + 1] = {1LL,
100000000000000000LL,
1000000000000000000LL};
+static inline bool isdigit(char value) { return std::isdigit(value) != 0; }
+
static void StringToInteger(const std::string& str, Decimal128* out) {
using std::size_t;
- DCHECK_NE(out, nullptr) << "Decimal128 output variable cannot be nullptr";
+ DCHECK_NE(out, NULLPTR) << "Decimal128 output variable cannot be NULLPTR";
DCHECK_EQ(*out, 0)
<< "When converting a string to Decimal128 the initial output must be 0";
@@ -189,7 +214,7 @@ static void StringToInteger(const std::string& str, Decimal128* out) {
Status Decimal128::FromString(const std::string& s, Decimal128* out, int* precision,
int* scale) {
- // Implements this regex: "(\\+?|-?)((0*)(\\d*))(\\.(\\d+))?";
+ // Implements this regex: "(\\+?|-?)((0*)(\\d*))(\\.(\\d+))?((E|e)(\\+|-)?\\d+)?";
if (s.empty()) {
return Status::Invalid("Empty string cannot be converted to decimal");
}
@@ -215,21 +240,21 @@ Status Decimal128::FromString(const std::string& s, Decimal128* out, int* precis
DCHECK_LT(charp, end);
// skip leading zeros
- charp = std::find_if_not(charp, end, [](char c) { return c == '0'; });
+ charp = std::find_if_not(charp, end, [](char value) { return value == '0'; });
// all zeros and no decimal point
if (charp == end) {
- if (out != nullptr) {
+ if (out != NULLPTR) {
*out = 0;
}
// Not sure what other libraries assign precision to for this case (this case of
// a string consisting only of one or more zeros)
- if (precision != nullptr) {
+ if (precision != NULLPTR) {
*precision = static_cast<int>(charp - numeric_string_start);
}
- if (scale != nullptr) {
+ if (scale != NULLPTR) {
*scale = 0;
}
@@ -238,7 +263,7 @@ Status Decimal128::FromString(const std::string& s, Decimal128* out, int* precis
std::string::const_iterator whole_part_start = charp;
- charp = std::find_if_not(charp, end, [](char c) { return std::isdigit(c) != 0; });
+ charp = std::find_if_not(charp, end, isdigit);
std::string::const_iterator whole_part_end = charp;
std::string whole_part(whole_part_start, whole_part_end);
@@ -269,14 +294,13 @@ Status Decimal128::FromString(const std::string& s, Decimal128* out, int* precis
std::string::const_iterator fractional_part_start = charp;
- // The rest must be digits, because if we have a decimal point it must be followed by
- // digits
+ // The rest must be digits or an exponent
if (charp != end) {
- charp = std::find_if_not(charp, end, [](char c) { return std::isdigit(c) != 0; });
+ charp = std::find_if_not(charp, end, isdigit);
// The while loop has ended before the end of the string which means we've hit a
- // character that isn't a base ten digit
- if (charp != end) {
+ // character that isn't a base ten digit or "E" for exponent
+ if (charp != end && *charp != 'E' && *charp != 'e') {
std::stringstream ss;
ss << "Found non base ten digit character '" << *charp
<< "' before the end of the string";
@@ -287,15 +311,55 @@ Status Decimal128::FromString(const std::string& s, Decimal128* out, int* precis
std::string::const_iterator fractional_part_end = charp;
std::string fractional_part(fractional_part_start, fractional_part_end);
- if (precision != nullptr) {
+ if (precision != NULLPTR) {
*precision = static_cast<int>(whole_part.size() + fractional_part.size());
}
- if (scale != nullptr) {
- *scale = static_cast<int>(fractional_part.size());
+ if (charp != end) {
+ // we must have an exponent, if this aborts then we have somehow not caught this and
+ // raised a proper error
+ DCHECK(*charp == 'E' || *charp == 'e');
+
+ ++charp;
+
+ const char value = *charp;
+ const bool starts_with_plus_or_minus = value == '+' || value == '-';
+
+ // we use this to construct the adjusted exponent integer later
+ std::string::const_iterator digit_start = charp;
+
+ // skip plus or minus
+ charp += starts_with_plus_or_minus;
+
+ // confirm that the rest of the characters are digits
+ charp = std::find_if_not(charp, end, isdigit);
+
+ if (charp != end) {
+ // we have something other than digits here
+ std::stringstream ss;
+ ss << "Found non decimal digit exponent value '" << *charp << "'";
+ return Status::Invalid(ss.str());
+ }
+
+ if (scale != NULLPTR) {
+ // compute the scale from the adjusted exponent
+ std::string adjusted_exponent_string(digit_start, end);
+ DCHECK(std::all_of(adjusted_exponent_string.cbegin() + starts_with_plus_or_minus,
+ adjusted_exponent_string.cend(), isdigit))
+ << "Non decimal digit character found in " << adjusted_exponent_string;
+ const auto adjusted_exponent =
+ static_cast<int32_t>(std::stol(adjusted_exponent_string));
+ const auto len = static_cast<int32_t>(whole_part.size() + fractional_part.size());
+
+ *scale = -adjusted_exponent + len - 1;
+ }
+ } else {
+ if (scale != NULLPTR) {
+ *scale = static_cast<int>(fractional_part.size());
+ }
}
- if (out != nullptr) {
+ if (out != NULLPTR) {
// zero out in case we've passed in a previously used value
*out = 0;
StringToInteger(whole_part + fractional_part, out);
--
To stop receiving notification emails like this one, please contact
['"commits@arrow.apache.org" <co...@arrow.apache.org>'].