Test : move tests for parse_string_decimal_native to parse_decimal #7177
Conversation
| 38, | ||
| 3, | ||
| ), | ||
| "0.127" | ||
| "0.126" |
There was a problem hiding this comment.
parse_decimal does not support rounding yet. it does truncate instead
| 38, | ||
| 2, | ||
| ), | ||
| "0.12" | ||
| ); | ||
| assert_eq!( | ||
| Decimal128Type::format_decimal( | ||
| parse_string_to_decimal_native::<Decimal128Type>(".1265", 2).unwrap(), | ||
| parse_decimal::<Decimal128Type>(".1265", 38, 2).unwrap(), |
There was a problem hiding this comment.
parse_decimal does not support rounding yet. it does truncate instead
| 38, | ||
| 2, | ||
| ), | ||
| "0.13" | ||
| "0.12" |
| 12300000_i128 | ||
| ); | ||
|
|
||
| // `parse_decimal` does not handle scale=0 correctly. will enable it as part of code change PR. | ||
| // assert_eq!(parse_decimal::<Decimal128Type>("123.45", 38, 0)?, 123_i128); |
There was a problem hiding this comment.
parse_decimal does not behave correctly when scale=0 when there are decimal digits in the original string. fix is in this PR #7179
There was a problem hiding this comment.
Thank you @himadripal -- i took a brief look and have a few questions
I am sorry for the delay in reviewing and the extra review, but I don't want to introduce another regression / need to do another release
| 12345000_i128 | ||
| ); | ||
|
|
||
| assert_eq!( | ||
| parse_string_to_decimal_native::<Decimal128Type>("123.4567891", 0)?, | ||
| //scale = 0 is not handled correctly in parse_decimal, next PR will fix it and enable this. |
There was a problem hiding this comment.
Doens't this comment out existing tests? Why would you reduce test coverage?
There was a problem hiding this comment.
But my point is that this PR actually changes what is tested (it isn't just a migration / refactor).
I expect a refactor to change how the code is structured but not change the coverage
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Now, we can close this PR, as I added additional test in #7179 to check the values generated by parse_string_to_decimal_native and parse_decimal are same. So no change in existing test or coverage.
| @@ -8416,92 +8417,92 @@ mod tests { | |||
| fn test_parse_string_to_decimal() { | |||
| assert_eq!( | |||
| Decimal128Type::format_decimal( | |||
| parse_string_to_decimal_native::<Decimal128Type>("123.45", 2).unwrap(), | |||
| parse_decimal::<Decimal128Type>("123.45", 38, 2).unwrap(), | |||
There was a problem hiding this comment.
This test is now testing a different function. I am not familar with the difference betwen parse_string_to_decimal and parse_decima
There was a problem hiding this comment.
Idea is to move from parse_string_to_decimal_native and to use parse_decimal while doing string to decimal casting here. Main reason is that parse_decimal has support for e-notation and is also being used in readers like (i.e arrow-csv, arrow-json and also performant, some discussion here
There was a problem hiding this comment.
so is your plan to remove the parse_string_to_decimal function?
There was a problem hiding this comment.
Yes, to be consistent, readers and cast will be using same function.
Which issue does this PR close?
Few important consideration -
parse_string_to_decimal_nativeparse_string_to_decimal_nativedoes not have support for e-notationparse_string_to_decimal_nativedoes rounding at scale, not truncateparse_decimalan existing method has e-notation support and use elsewhereparse_decimalparse_decimalto get support for e-notation.This PR is a first one to break up #6905 , this one only moves the existing
parse_string_to_decimal_nativetests to useparse_decimal. You can observe I changed the tests becauseparse_decimaldoes not have rounding support as is. Next PR, I'll introduce that change and change these tests again.Closes #.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?