pitrou commented on code in PR #49832:
URL: https://github.com/apache/arrow/pull/49832#discussion_r3396325662
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1763,9 +1769,10 @@ TEST(Decimal32Test, LeftShift) {
check(123, 16);
check(123, 30);
- ASSERT_EQ(Decimal32("1999999998"), Decimal32("999999999") << 1);
+ // results trimmed to kMaxPrecision (9) digits (GH-49817)
Review Comment:
The comment is a bit confusing, it implies that it's the left shift
operation that trims the digits.
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -708,14 +711,16 @@ TEST(Decimal128Test, PrintLargeNegativeValue) {
}
TEST(Decimal128Test, PrintMaxValue) {
- const std::string string_value("170141183460469231731687303715884105727");
+ // trimmed to kMaxPrecision (38) digits (GH-49817)
+ const std::string string_value("17014118346046923173168730371588410572");
Review Comment:
This is not testing the max value anymore (which is
99999999999999999999999999999999999999). Should we test both values using a
simple loop?
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1740,9 +1745,10 @@ TYPED_TEST(TestBasicDecimalFunctionality,
FitsInPrecision) {
ASSERT_TRUE(TypeParam(max_nines).FitsInPrecision(TypeParam::kMaxPrecision));
ASSERT_TRUE(TypeParam("-" +
max_nines).FitsInPrecision(TypeParam::kMaxPrecision));
- std::string max_zeros(TypeParam::kMaxPrecision, '0');
- ASSERT_FALSE(TypeParam("1" +
max_zeros).FitsInPrecision(TypeParam::kMaxPrecision));
- ASSERT_FALSE(TypeParam("-1" +
max_zeros).FitsInPrecision(TypeParam::kMaxPrecision));
+ // construct 10^kMaxPrecision without going through the string parser
(GH-49817)
+ TypeParam one_past_max = TypeParam::GetMaxValue(TypeParam::kMaxPrecision) +
1;
Review Comment:
Why not `max_nines + 1`?
##########
cpp/src/arrow/util/decimal_test.cc:
##########
@@ -1852,7 +1860,8 @@ TEST(Decimal64Test, LeftShift) {
ASSERT_EQ(Decimal64("1234567890123456"), Decimal64("1234567890123456") << 0);
ASSERT_EQ(Decimal64("2469135780246912"), Decimal64("1234567890123456") << 1);
- ASSERT_EQ(Decimal64("6917529027641081856"), Decimal64("1234567890123456") <<
55);
+ // shift 9 keeps the result within kMaxPrecision (18 digits) (GH-49817)
Review Comment:
Similarly, not sure this comment is informative to the reader.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
