You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@arrow.apache.org by "coussens (via GitHub)" <gi...@apache.org> on 2024/09/05 01:06:26 UTC
[I] stringr binding for `str_sub()` silently mishandles 'negative' index values [arrow]
coussens opened a new issue, #43960:
URL: https://github.com/apache/arrow/issues/43960
### Describe the bug, including details regarding any error messages, version, and platform.
I noticed some unusual behavior behavior when attempting to use 'negative' start/end values (i.e. counting from the end of the string) when using `str_sub()` in arrow. I've included a few examples below, contrasting how `str_sub` behaves with tibbles in R and arrow tables:
``` r
library(arrow)
library(tidyverse)
library(reprex)
str_tbl <- tibble(my_string = 'abcde')
str_tbl_a <- as_arrow_table(str_tbl)
# example #1: extract first through second-to-last characters from string
# works fine with a tibble
str_tbl %>% mutate(my_substring = str_sub(my_string, start = 1, end = -2))
#> # A tibble: 1 × 2
#> my_string my_substring
#> <chr> <chr>
#> 1 abcde abcd
# but with arrow: ruh-roh -- missing all characters
str_tbl_a %>% mutate(my_substring = str_sub(my_string, start = 1, end = -2)) %>% collect()
#> # A tibble: 1 × 2
#> my_string my_substring
#> <chr> <chr>
#> 1 abcde ""
# example #2: extract third-to-last through second-to-last characters from string
# works fine with a tibble
str_tbl %>% mutate(my_substring = str_sub(my_string, start = -3, end = -2))
#> # A tibble: 1 × 2
#> my_string my_substring
#> <chr> <chr>
#> 1 abcde cd
# but with arrow: ruh-roh -- missing a character
str_tbl_a %>% mutate(my_substring = str_sub(my_string, -3, -2)) %>% collect()
#> # A tibble: 1 × 2
#> my_string my_substring
#> <chr> <chr>
#> 1 abcde c
# example #3: extract third-to-last through last characters from string
# works fine with a tibble
str_tbl %>% mutate(my_substring = str_sub(my_string, start = -3, end = -1))
#> # A tibble: 1 × 2
#> my_string my_substring
#> <chr> <chr>
#> 1 abcde cde
# but with arrow: bizarrely, this is also fine
str_tbl_a %>% mutate(my_substring = str_sub(my_string, -3, -1)) %>% collect()
#> # A tibble: 1 × 2
#> my_string my_substring
#> <chr> <chr>
#> 1 abcde cde
```
<sup>Created on 2024-09-05 with [reprex v2.1.1](https://reprex.tidyverse.org)</sup>
### Component(s)
R
--
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: issues-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [R] stringr binding for `str_sub()` silently mishandles negative start/stop values [arrow]
Posted by "coussens (via GitHub)" <gi...@apache.org>.
coussens commented on issue #43960:
URL: https://github.com/apache/arrow/issues/43960#issuecomment-2344576504
@thisisnic totally agree that the snippet you point out is the culprit for the behavior in Example 1 above. I think the conditional logic could instead be changed to the following in order to avoid returning empty strings for negative values of `end`:
```
if(end < start & end > 0) {
end <- 0
start <- 0
}
```
Upon looking at the source code (sorry I hadn't taken the time to examine before), I think more problems in addition to this (but please correct me if I'm wrong!).
I believe the `end` argument in the C++ function `utf8_slice_codeunits` is _exclusive_, so won't include `my_string[end]` in the returned substring. For positive `end` values, this poses no issue, since in C++ is 0-based and R is 1-based, cancelling each other out. Note that for positive `start` values, the [below snippet in the source code](https://github.com/apache/arrow/blob/main/r/R/dplyr-funcs-string.R#L573) addresses this:
```
# subtract 1 from `start` because C++ is 0-based and R is 1-based
# str_sub treats a `start` value of 0 or 1 as the same thing so don't subtract 1 when `start` == 0
# when `start` < 0, both str_sub and utf8_slice_codeunits count backwards from the end
if (start > 0) {
start <- start - 1L
}
```
For negative `start` values, this isn't necessary, as noted in source comments above. But that leaves the issue of negative `end` values unsolved. This explains why in my Example 2 above `arrow::str_sub` fails to include my_string[-2] (i.e. only returns 'c' instead of the correct 'cd').
However, for just the [default] case when `end == -1`, this problem is incidentally fixed by [this code](https://github.com/apache/arrow/blob/main/r/R/dplyr-funcs-string.R#L567):
```
# In stringr::str_sub, an `end` value of -1 means the end of the string, so
# set it to the maximum integer to match this behavior
if (end == -1) {
end <- .Machine$integer.max
}
```
which explains why `arrow::str_sub` works fine (as in my Example 3 above) when `end == -1`, but not when `end < -1` (which confused me at first).
I'm happy to create a MR that should fix this, with the warning that while I've been working with R for quite a while, I'm a total newb when it comes to the collaborative development side of things (@thisisnic thanks so much for your https://github.com/forwards/first-contributions tutorial!).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [R] stringr binding for `str_sub()` silently mishandles negative start/stop values [arrow]
Posted by "coussens (via GitHub)" <gi...@apache.org>.
coussens commented on issue #43960:
URL: https://github.com/apache/arrow/issues/43960#issuecomment-2351827562
OK, sounds good -- I'll write a PR for this!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [R] stringr binding for `str_sub()` silently mishandles negative start/stop values [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic closed issue #43960: [R] stringr binding for `str_sub()` silently mishandles negative start/stop values
URL: https://github.com/apache/arrow/issues/43960
--
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: issues-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [R] stringr binding for `str_sub()` silently mishandles negative start/stop values [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on issue #43960:
URL: https://github.com/apache/arrow/issues/43960#issuecomment-2365130430
Issue resolved by pull request 44141
https://github.com/apache/arrow/pull/44141
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [R] stringr binding for `str_sub()` silently mishandles negative start/stop values [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on issue #43960:
URL: https://github.com/apache/arrow/issues/43960#issuecomment-2336716153
Can confirm that I can replicate this with the dev version of Arrow too.
In our implementation of `str_sub()` there's a bit that looks like this:
```
# An end value lower than a start value returns an empty string in
# stringr::str_sub so set end to 0 here to match this behavior
if (end < start) {
end <- 0
}
```
That might not be quite right, so we'll need to add some more tests and revisit the logic here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [R] stringr binding for `str_sub()` silently mishandles negative start/stop values [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on issue #43960:
URL: https://github.com/apache/arrow/issues/43960#issuecomment-2351129684
I'd love if you'd be happy to contribute a PR! And don't worry, we're friendly around here - we don't expect everything t be perfect first time round (my PRs still aren't ;) ) and will help you through the process of become an arrow contributor!
Thanks for investigating this so thoroughly, and look forward to reviewing a PR! :D
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org