Fix negative decimal string #5128
Conversation
| @@ -8304,6 +8331,8 @@ mod tests { | |||
| Some(""), | |||
| Some(" "), | |||
| None, | |||
| Some("-1.23499999"), | |||
| Some("-1.23599999"), | |||
There was a problem hiding this comment.
Perhaps worth testing -0.001 or similar?
There was a problem hiding this comment.
-+123.45 should be failed, but pass.
There was a problem hiding this comment.
The result for -123 is wrong, which ignores the minus sign.
There was a problem hiding this comment.
Added the test case (and others).
arrow-cast/src/cast.rs
Outdated
| let decimals = if parts.len() == 2 { parts[1] } else { "" }; | ||
|
|
||
| let (decimals, negative) = if parts.len() == 2 { | ||
| (parts[1], integers.starts_with('-')) |
There was a problem hiding this comment.
Should this after the trim_start_matches above? I think this will parse 000-1.343
arrow-cast/src/cast.rs
Outdated
| }; | ||
|
|
||
| let integers = if negative { | ||
| integers.trim_start_matches('-') |
There was a problem hiding this comment.
Should this be trim_start_matches or just a single strip_prefix?
arrow-cast/src/cast.rs
Outdated
| }; | ||
|
|
||
| if negative { | ||
| format!("{}", integers.sub_wrapping(adjusted)) |
There was a problem hiding this comment.
Not related to this PR, but I wonder if we need to format to a string and then convert back
There was a problem hiding this comment.
Hmm, I forgot why we did the conversion at first. Maybe we can get rid of it. As it is not part of this PR, I will do it later.
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Co-authored-by: Raphael Taylor-Davies <[email protected]>
| Some("1.-23499999"), | ||
| Some("-1.-23499999"), | ||
| Some("--1.23499999"), |
There was a problem hiding this comment.
Why results of these cases are null instead of error?
There was a problem hiding this comment.
When CastOptions.safe is true (default), cast failure returns NULL.
Which issue does this PR close?
Closes #5127.
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?