Jefffrey commented on code in PR #8700:
URL: https://github.com/apache/arrow-rs/pull/8700#discussion_r2676341440
##########
arrow-cast/src/parse.rs:
##########
@@ -741,72 +741,53 @@ fn parse_e_notation<T: DecimalType>(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
- let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
- // skip to point or exponent index
- let mut bs;
- if fractionals > 0 {
- // it's a fraction, so the point index needs to be skipped, so +1
- bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
- } else {
- // it's actually an integer that is already written into the result,
so let's skip on to e
- bs = s.as_bytes().iter().skip(index);
- }
+ // skip to the exponent index directly or just after any processed
fractionals
+ let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
- while let Some(b) = bs.next() {
+ // complete parsing of any unprocessed fractionals up to the exponent
Review Comment:
I must admit I'm confused by this line; above we state we skip to the
exponent or after any processed fractionals, but here we say we parse
unprocessed fractionals somehow?
After checking the code it seems it corresponds to this check:
https://github.com/splitgraph/arrow-rs/blob/3dbfe0d16447d4344ef1f6caeb2befddc0e91aed/arrow-cast/src/parse.rs#L900-L905
Where we previously skipped fractionals for exceeding the scale, but now we
include them since we parse the exponent. I feel we should capture this detail
here.
##########
arrow-cast/src/parse.rs:
##########
@@ -741,72 +741,53 @@ fn parse_e_notation<T: DecimalType>(
let mut exp: i16 = 0;
let base = T::Native::usize_as(10);
- let mut exp_start: bool = false;
// e has a plus sign
let mut pos_shift_direction: bool = true;
- // skip to point or exponent index
- let mut bs;
- if fractionals > 0 {
- // it's a fraction, so the point index needs to be skipped, so +1
- bs = s.as_bytes().iter().skip(index + fractionals as usize + 1);
- } else {
- // it's actually an integer that is already written into the result,
so let's skip on to e
- bs = s.as_bytes().iter().skip(index);
- }
+ // skip to the exponent index directly or just after any processed
fractionals
+ let mut bs = s.as_bytes().iter().skip(index + fractionals as usize);
Review Comment:
If we're refactoring perhaps we should change the type of fractions to be
unsigned? It was a little confusing to have it be signed to me as I had to
verify that your refactor to collapse the if/else made sense since fractionals
was never negative (and the `+ 1` in the if block is accounted for by adjusting
`index` at the callsite)
--
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]
