scovich commented on code in PR #9421:
URL: https://github.com/apache/arrow-rs/pull/9421#discussion_r2831258022
##########
arrow-cast/src/parse.rs:
##########
@@ -826,7 +826,9 @@ fn parse_e_notation<T: DecimalType>(
}
if exp < 0 {
- result = result.div_wrapping(base.pow_wrapping(-exp as _));
+ let divisor = base.pow_wrapping(-exp as _);
+ let half_divisor = divisor.div_wrapping(T::Native::usize_as(2));
+ result = result.add_wrapping(half_divisor).div_wrapping(divisor);
Review Comment:
It seems like this could cause a near-max value to overflow before the
division could succeed? e.g. for u8, 255/10 = 25 but (255+5)/10 overflows on
add. I have vague memories that other parts of the decimal code have a trick
for avoiding the problem. I don't remember what trick or where, but it's likely
in decimal casting/scaling code.
Either way, we should have a unit test that verifies we don't introduce new
"intermediate" overflows like that. A true overflow due to rounding up with
carry-over would be allowed, e.g. parsing 99.9 to a (2, 0) decimal would
overflow because 100 isn't representable.
##########
arrow-cast/src/parse.rs:
##########
@@ -964,6 +974,8 @@ pub fn parse_decimal<T: DecimalType>(
return Err(ArrowError::ParseError(format!(
"parse decimal overflow ({s})"
)));
+ } else if round_up {
+ result = result.add_wrapping(T::Native::usize_as(1));
Review Comment:
nit:
[T::Native::ONE](https://docs.rs/arrow/latest/arrow/array/trait.ArrowNativeTypeOp.html#associatedconstant.ONE)?
##########
arrow-cast/src/parse.rs:
##########
@@ -915,6 +919,12 @@ pub fn parse_decimal<T: DecimalType>(
// We have processed all the digits that we need. All
that
// is left is to validate that the rest of the string
contains
// valid digits.
Review Comment:
Worth updating the comment?
##########
arrow-cast/src/parse.rs:
##########
@@ -915,6 +919,12 @@ pub fn parse_decimal<T: DecimalType>(
// We have processed all the digits that we need. All
that
// is left is to validate that the rest of the string
contains
// valid digits.
+ if !rounding_checked {
+ if *b >= b'5' {
+ round_up = true;
+ }
Review Comment:
nit: the branch likely costs more than it's worth (both in LoC and in CPU
cycles)
```suggestion
round_up = *b >= b'5';
```
--
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]
