fix: Enable cast string to int tests and fix compatibility issue#453
Conversation
| for i in 0..len { | ||
| if $array.is_null(i) { | ||
| cast_array.append_null() | ||
| } else if let Some(cast_value) = $cast_method($array.value(i).trim(), $eval_mode)? { |
There was a problem hiding this comment.
We originally trimmed the input strings here, so error messages did not have access to the original inputs
|
|
||
| if state == State::ParseSignAndDigits { | ||
| if !parsed_sign { | ||
| for (i, ch) in trimmed_str.char_indices() { |
There was a problem hiding this comment.
We process the trimmed string here and we no longer need the logic for skipping leading and trailing whitespace
| parse_sign_and_digits = false; | ||
| continue; | ||
| } else { | ||
| return none_or_err(eval_mode, type_name, str); |
There was a problem hiding this comment.
error messages refer to the original input string, not the trimmed version
kazuyukitanimura
left a comment
kazuyukitanimura
left a comment
There was a problem hiding this comment.
LGTM pending CI
|
So do you thing the perf improvement is because we are no longer trimming? |
We are still trimming, but we are no longer performing the redundant conditional logic in the main loop to skip leading and trailing whitespace. |
4 participants
Which issue does this PR close?
Closes #431
Rationale for this change
Enable cast string to int as a compatible expression.
Also, there is a performance improvement with these changes:
What changes are included in this PR?
How are these changes tested?
Enabled the tests that were previously disabled