You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/12/17 19:16:41 UTC
[GitHub] [arrow] arw2019 opened a new pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
arw2019 opened a new pull request #8955:
URL: https://github.com/apache/arrow/pull/8955
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#discussion_r551389414
##########
File path: cpp/src/arrow/util/decimal_test.cc
##########
@@ -169,6 +169,11 @@ TEST(DecimalTest, TestDecimalStringAndBytesRoundTrip) {
ASSERT_EQ(expected, result);
}
+TEST(DecimalTest, TestOverflow) {
+ std::string invalid_value("1e100");
Review comment:
Also test a negative exponent and boundary values such as 38 and -38?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-747646366
https://issues.apache.org/jira/browse/ARROW-9948
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kiszk commented on a change in pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
kiszk commented on a change in pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#discussion_r546328882
##########
File path: cpp/src/arrow/util/basic_decimal.cc
##########
@@ -949,33 +949,31 @@ DecimalStatus BasicDecimal128::Rescale(int32_t original_scale, int32_t new_scale
void BasicDecimal128::GetWholeAndFraction(int scale, BasicDecimal128* whole,
BasicDecimal128* fraction) const {
- DCHECK_GE(scale, 0);
- DCHECK_LE(scale, 38);
+ DCHECK_EQ(ValidateScale(scale), true);
BasicDecimal128 multiplier(ScaleMultipliers[scale]);
auto s = Divide(multiplier, whole, fraction);
DCHECK_EQ(s, DecimalStatus::kSuccess);
}
+bool BasicDecimal128::ValidateScale(int32_t scale) { return (std::abs(scale) <= 38); }
Review comment:
This function can accept `-38 <= scale <= 38` while the original code accepted `0 <= scale <= 38`.
Is this change expected? If so, it would be good to leave a comment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] arw2019 commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
arw2019 commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-759253925
> @arw2019 We plan to release 3.0 soon. Would you like to finish this PR?
@pitrou sorry about the delay! Even though there's not a huge amount to do here unfortunately I won't find the time to do this likely until the weekend.
I'll push this over to 4.0 on JIRA so it's not blocking the release (but can be changed back to 3.0 in case I'm able to respond before the RC is cut).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-820535098
Ping @arw2019 -- is this something you're still planning to work on?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] arw2019 commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
arw2019 commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-785898572
> @arw2019 Are you still interested in finishing this?
@pitrou sorry for the delay. yes - will get to it today or over the weekend
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] arw2019 commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
arw2019 commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-820844173
@pitrou sorry about the delay! Closing to clear the queue - if the ticket is still open when I get the chance to look I'll reopen
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] arw2019 commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
arw2019 commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-754383958
Thanks @pitrou @kiszk for reviewing! Planning to push an update in the next day or so
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-758755125
@arw2019 We plan to release 3.0 soon. Would you like to finish this PR?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] arw2019 closed pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
arw2019 closed pull request #8955:
URL: https://github.com/apache/arrow/pull/8955
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#issuecomment-785889550
@arw2019 Are you still interested in finishing this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #8955: ARROW-9948: [C++] in Decimal128::FromString raise when scale is out of bounds
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #8955:
URL: https://github.com/apache/arrow/pull/8955#discussion_r551388197
##########
File path: cpp/src/arrow/util/basic_decimal.cc
##########
@@ -949,33 +949,31 @@ DecimalStatus BasicDecimal128::Rescale(int32_t original_scale, int32_t new_scale
void BasicDecimal128::GetWholeAndFraction(int scale, BasicDecimal128* whole,
BasicDecimal128* fraction) const {
- DCHECK_GE(scale, 0);
- DCHECK_LE(scale, 38);
+ DCHECK_EQ(ValidateScale(scale), true);
BasicDecimal128 multiplier(ScaleMultipliers[scale]);
auto s = Divide(multiplier, whole, fraction);
DCHECK_EQ(s, DecimalStatus::kSuccess);
}
+bool BasicDecimal128::ValidateScale(int32_t scale) { return (std::abs(scale) <= 38); }
Review comment:
Especially as `ScaleMultipliers` is then going to be indexed with a negative value above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org