You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/29 22:17:53 UTC
[GitHub] [arrow] pachadotdev opened a new pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub() - JUST STR SUB SKETCH
pachadotdev opened a new pull request #10624:
URL: https://github.com/apache/arrow/pull/10624
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r663049290
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
Review comment:
Please can you split these out into separate tests? It makes it easier to follow and fix issues.
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
Review comment:
```suggestion
```
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
Review comment:
No need to set up string "Apache Arrow" via variable `foo` as it's already been set up as variable `x`.
```suggestion
bar1 = substr(x, 1, 6), # Apache
bar2 = substr(x, 0, 6), # Apache
bar3 = substr(x, -1, 6), # Apache
bar4 = substr(x, 6, 1), # ""
bar5 = substr(x, -1, -2), # ""
bar6 = substr(x, 8, 12) # Arrow
```
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r670449282
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,68 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ # substr treats values as if they're on a continous number line, so values
+ # 0 are effectively blank characters - set `start` to 1 here so Arrow mimics
+ # this behavior
+ if (start <= 0) {
+ start <- 1
+ }
+
+ # if `stop` is lower than `start`, this is invalid, so set `stop` to
+ # 0 so that an empty string will be returned
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- nse_funcs$substr
+
+nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "`end` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (end == -1) {
+ end <- .Machine$integer.max
+ }
+
+ if (end < start) {
+ end <- 0
+ }
+
+
+ if (start > 0) {
+ start <- start - 1L
Review comment:
Thanks, good to know, will update with 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
[GitHub] [arrow] pachadotdev commented on pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
pachadotdev commented on pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#issuecomment-878423203
> This needs tests. I'd like this included in 5.0 if possible.
thx, I re-added some of the tests
--
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
[GitHub] [arrow] nealrichardson commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r669913235
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
Review comment:
Can you explain these? It's not obvious why this is correct, and the base R code and docs don't discuss these cases.
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- function(text, first, last = 1000000L) {
Review comment:
You could just implement this one by calling nse_funcs$substr. The validation messages won't be exactly right because the argument names are different, but per the docs this function is only kept for compatibility with S, so I don't think it's a big deal.
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- function(text, first, last = 1000000L) {
+ assert_that(
+ length(first) == 1,
+ msg = "`first` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(last) == 1,
+ msg = "`last` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (first <= 0) {
+ first <- 1
+ }
+
+ if (last < first) {
+ last <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ text,
+ options = list(start = first - 1L, stop = last)
+ )
+}
+
+nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "`end` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start == 0) start <- 1
+
+ if (end == -1) end <- .Machine$integer.max
+
+ if (end < start) end <- 0
+
+ if (start > 0) start <- start - 1L
Review comment:
Why does this version subtract 1 up here but the others don't? Why only if start > 0? Is start <= 0 valid?
##########
File path: r/R/dplyr-functions.R
##########
@@ -391,7 +470,7 @@ nse_funcs$str_split <- function(string, pattern, n = Inf, simplify = FALSE) {
string,
options = list(
pattern =
- opts$pattern,
+ opts$pattern,
Review comment:
I know this is just linting but IDK why opts$pattern is on its own line instead of next to = above it.
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
Review comment:
Why do we only subtract 1 from start but not stop?
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- function(text, first, last = 1000000L) {
+ assert_that(
+ length(first) == 1,
+ msg = "`first` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(last) == 1,
+ msg = "`last` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (first <= 0) {
+ first <- 1
+ }
+
+ if (last < first) {
+ last <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ text,
+ options = list(start = first - 1L, stop = last)
+ )
+}
+
+nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "`end` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start == 0) start <- 1
Review comment:
Please use braces even for short if statements
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -909,5 +896,206 @@ test_that("str_pad", {
collect(),
df
)
+})
+
+test_that("substr", {
+ df <- tibble(x = "Apache Arrow")
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 1, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 0, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, -1, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 6, 1)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, -1, -2)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 9, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 1, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 8, 12)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, -5, -1)) %>%
+ collect(),
+ df
+ )
+})
+
+test_that("substring", {
Review comment:
If you made substring just call substr then you could delete all but one of these tests.
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r670256573
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
Review comment:
we don't need to subtract 1 from `end` as R counts inclusively (i.e. returned string includes the item at position `end`) whereas C++ counts exclusively (i.e. returned string includes up to the item at position `end` which effectively cancels out the difference due to indexing
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r662899545
##########
File path: r/R/expression.R
##########
@@ -32,6 +32,8 @@
"str_to_upper" = "utf8_upper",
"str_reverse" = "utf8_reverse",
# str_trim is defined in dplyr-functions.R
+ # str_sub is defined in dplyr-functions.R
+ # substr is defined in dplyr-functions.R
"year" = "year",
Review comment:
Should we mention `substring` here as well?
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
Review comment:
No need to set up string "Apache Arrow" via variable `foo` as it's already been set up as variable `x`.
```suggestion
bar1 = substr(x, 1, 6), # Apache
bar2 = substr(x, 0, 6), # Apache
bar3 = substr(x, -1, 6), # Apache
bar4 = substr(x, 6, 1), # ""
bar5 = substr(x, -1, -2), # ""
bar6 = substr(x, 8, 12) # Arrow
```
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +280,45 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "Start of length != 1 not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "End of length != 1 not supported in Arrow"
+ )
+
+ if (start > stop) {
+ start <- 0
+ stop <- 0
+ } else {
+ start <- max(0, start - 1)
+ stop <- max(0, stop)
+ start_stop <- c(min(start, stop), max(start, stop))
+ start <- start_stop[1]
+ stop <- start_stop[2]
+ }
Review comment:
I'm a little confused here - please can you give me an example of values that could be supplied as `start` and `stop` which would make it necessary to use `min()` and `max()` here to put the values back in the correct order?
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
Review comment:
Please can you split these out into separate tests? It makes it easier to follow and fix issues.
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
Review comment:
```suggestion
```
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
Review comment:
Please can you split these out into separate tests? It makes it easier to follow and fix issues.
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
Review comment:
```suggestion
```
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
Review comment:
No need to set up string "Apache Arrow" via variable `foo` as it's already been set up as variable `x`.
```suggestion
bar1 = substr(x, 1, 6), # Apache
bar2 = substr(x, 0, 6), # Apache
bar3 = substr(x, -1, 6), # Apache
bar4 = substr(x, 6, 1), # ""
bar5 = substr(x, -1, -2), # ""
bar6 = substr(x, 8, 12) # Arrow
```
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substring(foo, 1, 6), # Apache
+ bar2 = substring(foo, 0, 6), # Apache
+ bar3 = substring(foo, -1, 6), # Apache
+ bar4 = substring(foo, 6, 1), # ""
+ bar5 = substring(foo, -1, -2), # ""
+ bar6 = substring(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = str_sub(foo, 1, 6), # Apache
+ bar2 = str_sub(foo, 0, 6), # Apache
+ bar3 = str_sub(foo, -1, 6), # Apache
+ bar4 = str_sub(foo, 6, 1), # ""
+ bar5 = str_sub(foo, -1, -2), # ""
+ bar6 = str_sub(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
+ collect(),
+ df
+ )
+})
Review comment:
```suggestion
test_that("substrings", {
df <- tibble(
x = "Apache Arrow"
)
# substr
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 0, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, -1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 6, 1)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, -1, -2)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 8, 12)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, -5, -1)) %>%
collect(),
df
)
# substring
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 0, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, -1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 6, 1)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, -1, -2)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 8, 12)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, -5, -1)) %>%
collect(),
df
)
# str_sub
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 0, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, -1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 6, 1)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, -1, -2)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 8, 12)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, -5, -1)) %>%
collect(),
df
)
})
```
--
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
[GitHub] [arrow] pachadotdev commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
pachadotdev commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r664559978
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +280,45 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "Start of length != 1 not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "End of length != 1 not supported in Arrow"
+ )
+
+ if (start > stop) {
+ start <- 0
+ stop <- 0
+ } else {
+ start <- max(0, start - 1)
+ stop <- max(0, stop)
+ start_stop <- c(min(start, stop), max(start, stop))
+ start <- start_stop[1]
+ stop <- start_stop[2]
+ }
Review comment:
it's more a verification than a formal requirement
--
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
[GitHub] [arrow] pachadotdev commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
pachadotdev commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r668221668
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -868,42 +868,76 @@ test_that("str_like", {
})
test_that("str_pad", {
-
+
df <- tibble(x = c("Foo and bar", "baz and qux and quux"))
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 31)) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 30, side = "right")) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 31, side = "left", pad = "+")) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 10, side = "left", pad = "+")) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 31, side = "both")) %>%
collect(),
df
)
-
+})
+
+test_that("substr, substring, str_sub", {
+
+ df <- tibble(x1 = "Apache Arrow")
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ x2 = substr(x1, 1, 6), # Apache
+ x3 = substr(x1, 8, 12) # Arrow
+ ) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ x2 = substring(x1, 1, 6), # Apache
+ x3 = substring(x1, 8, 12) # Arrow
+ ) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ x2 = str_sub(x1, 1, 6), # Apache
+ x3 = str_sub(x1, 8, 12) # Arrow
+ ) %>%
+ collect(),
+ df
+ )
})
Review comment:
yes !! i have no idea what happened here, but i'll re-add or re-write those
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r669142885
##########
File path: r/src/compute.cpp
##########
@@ -316,6 +316,19 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
return std::make_shared<Options>(max_splits, reverse);
}
+ if (func_name == "utf8_slice_codeunits") {
+ using Options = arrow::compute::SliceOptions;
+ int64_t start = 1;
+ int64_t stop = -1;
Review comment:
Sorry, I didn't explain this properly, my fault! What I mean is that here stop has been set to `-1` but the default C++ value isn't `-1`, so the default value here probably shouldn't be `-1` either.
Check out this line of code here that shows the default value of `stop` in the C++: https://github.com/apache/arrow/blob/7114c4b6fdb639b3500d77cfd66649af8c5c5e6b/cpp/src/arrow/compute/api_scalar.h#L205-L206
The default value of `stop` is `std::numeric_limits<int64_t>::max()` which is the biggest int64. So if you don't supply a value for `stop` and just use the default, this allows you to slice to the end of the string.
In some of the other functions in this file, this line has been used to set the default values to the ones from the C++ code:
`std::make_shared<Options>(Options::Defaults());`
Maybe instead of manually setting the value of stop to `-1`, if you use the line above, it might make it easier to fix some of the failing tests as now you'll be able to slice to the end of a string. If this doesn't make sense, let me know!
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r670256189
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- function(text, first, last = 1000000L) {
+ assert_that(
+ length(first) == 1,
+ msg = "`first` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(last) == 1,
+ msg = "`last` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (first <= 0) {
+ first <- 1
+ }
+
+ if (last < first) {
+ last <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ text,
+ options = list(start = first - 1L, stop = last)
+ )
+}
+
+nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "`end` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start == 0) start <- 1
+
+ if (end == -1) end <- .Machine$integer.max
+
+ if (end < start) end <- 0
+
+ if (start > 0) start <- start - 1L
Review comment:
Hmm, this also makes setting `start` to 1 if it's 0 totally redundant so I'll remove that too
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r664603120
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +280,45 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "Start of length != 1 not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "End of length != 1 not supported in Arrow"
+ )
+
+ if (start > stop) {
+ start <- 0
+ stop <- 0
+ } else {
+ start <- max(0, start - 1)
+ stop <- max(0, stop)
+ start_stop <- c(min(start, stop), max(start, stop))
+ start <- start_stop[1]
+ stop <- start_stop[2]
+ }
Review comment:
Sorry, I'm still confused, could you explain a bit more? Just that it seems a bit dangerous to be swapping parameters around unless we have reason to, right?
--
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
[GitHub] [arrow] thisisnic commented on pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#issuecomment-880113887
This PR now also contains some unrelated styling changes as I ran styler on the files I changed before pushing my changes.
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub() - JUST STR SUB SKETCH
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#issuecomment-870956307
https://issues.apache.org/jira/browse/ARROW-12992
--
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
[GitHub] [arrow] thisisnic commented on pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#issuecomment-880527207
@nealrichardson - have made updates, when you get the chance, please could you re-review 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
[GitHub] [arrow] nealrichardson commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r670405245
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,68 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ # substr treats values as if they're on a continous number line, so values
+ # 0 are effectively blank characters - set `start` to 1 here so Arrow mimics
+ # this behavior
+ if (start <= 0) {
+ start <- 1
+ }
+
+ # if `stop` is lower than `start`, this is invalid, so set `stop` to
+ # 0 so that an empty string will be returned
Review comment:
```suggestion
# 0 so that an empty string will be returned (consistent with base::substr())
```
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,68 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ # substr treats values as if they're on a continous number line, so values
+ # 0 are effectively blank characters - set `start` to 1 here so Arrow mimics
+ # this behavior
+ if (start <= 0) {
+ start <- 1
+ }
+
+ # if `stop` is lower than `start`, this is invalid, so set `stop` to
+ # 0 so that an empty string will be returned
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- nse_funcs$substr
+
+nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "`end` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (end == -1) {
+ end <- .Machine$integer.max
+ }
+
+ if (end < start) {
+ end <- 0
+ }
+
+
Review comment:
```suggestion
```
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -909,5 +895,176 @@ test_that("str_pad", {
collect(),
df
)
+})
+
+test_that("substr", {
+ df <- tibble(x = "Apache Arrow")
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 1, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 0, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, -1, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 6, 1)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, -1, -2)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 9, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 1, 6)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, 8, 12)) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(y = substr(x, -5, -1)) %>%
+ collect(),
+ df
+ )
+
+ expect_error(
+ nse_funcs$substr("Apache Arrow", c(1, 2), 3),
+ "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ expect_error(
+ nse_funcs$substr("Apache Arrow", 1, c(2, 3)),
+ "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+})
+
+test_that("substring", {
+ df <- tibble(x = "Apache Arrow")
Review comment:
```suggestion
# nse_funcs$substring just calls nse_funcs$substr, tested extensively above
df <- tibble(x = "Apache Arrow")
```
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,68 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ # substr treats values as if they're on a continous number line, so values
+ # 0 are effectively blank characters - set `start` to 1 here so Arrow mimics
+ # this behavior
+ if (start <= 0) {
+ start <- 1
+ }
+
+ # if `stop` is lower than `start`, this is invalid, so set `stop` to
+ # 0 so that an empty string will be returned
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- nse_funcs$substr
+
+nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "`end` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (end == -1) {
+ end <- .Machine$integer.max
+ }
+
+ if (end < start) {
+ end <- 0
+ }
+
+
+ if (start > 0) {
+ start <- start - 1L
Review comment:
These deserve explanatory comments, particularly noting the differences in behavior among base::substr, stringr::str_sub, and arrow's utf8_slice_codeunits. Basically a concise version of what you answered me in the PR review. (It's a good rule of thumb that if I ask you why something is a certain way because it wasn't obvious to me, the answer should probably be a code comment and not (just) a PR comment--if it's not obvious to the reader now, it definitely won't be obvious to us in 6 months or whenever we have to revise this code and wonder why things are munged a certain way.)
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,68 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ # substr treats values as if they're on a continous number line, so values
+ # 0 are effectively blank characters - set `start` to 1 here so Arrow mimics
+ # this behavior
+ if (start <= 0) {
+ start <- 1
+ }
+
+ # if `stop` is lower than `start`, this is invalid, so set `stop` to
+ # 0 so that an empty string will be returned
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- nse_funcs$substr
Review comment:
This won't exactly work because the arguments are named differently
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
Review comment:
This deserves an explanatory comment
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r667773505
##########
File path: r/R/dplyr-functions.R
##########
@@ -286,8 +286,8 @@ nse_funcs$substr <- function(string, start, stop) {
msg = "Start of length != 1 not supported in Arrow"
)
assert_that(
- length(end) == 1,
- msg = "End of length != 1 not supported in Arrow"
+ length(stop) == 1,
+ msg = "Stop of length != 1 not supported in Arrow"
Review comment:
```suggestion
msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
```
##########
File path: r/R/dplyr-functions.R
##########
@@ -313,10 +313,33 @@ nse_funcs$substring <- function(text, first, last = 1000000L) {
}
nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
- if (start < 0 || end < 0) {
- warning("Negative counts not yet implemented for strings")
+ if(end <= -1) end <- .Machine$integer.max
+
+ assert_that(
+ length(start) == 1,
+ msg = "Start of length != 1 not supported in Arrow"
Review comment:
```suggestion
msg = "`start` must be length 1 - other lengths are not supported in Arrow"
```
##########
File path: r/R/dplyr-functions.R
##########
@@ -313,10 +313,33 @@ nse_funcs$substring <- function(text, first, last = 1000000L) {
}
nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
- if (start < 0 || end < 0) {
- warning("Negative counts not yet implemented for strings")
+ if(end <= -1) end <- .Machine$integer.max
+
+ assert_that(
+ length(start) == 1,
+ msg = "Start of length != 1 not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "end of length != 1 not supported in Arrow"
Review comment:
```suggestion
msg = "`end` must be length 1 - other lengths are not supported in Arrow"
```
##########
File path: r/src/compute.cpp
##########
@@ -316,6 +316,19 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
return std::make_shared<Options>(max_splits, reverse);
}
+ if (func_name == "utf8_slice_codeunits") {
+ using Options = arrow::compute::SliceOptions;
+ int64_t start = 1;
+ int64_t stop = -1;
Review comment:
I think this still may need updating to reflect the default value of stop supplied by C++
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -868,42 +868,76 @@ test_that("str_like", {
})
test_that("str_pad", {
-
+
df <- tibble(x = c("Foo and bar", "baz and qux and quux"))
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 31)) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 30, side = "right")) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 31, side = "left", pad = "+")) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 10, side = "left", pad = "+")) %>%
collect(),
df
)
-
+
expect_dplyr_equal(
input %>%
mutate(x = str_pad(x, width = 31, side = "both")) %>%
collect(),
df
)
-
+})
+
+test_that("substr, substring, str_sub", {
+
+ df <- tibble(x1 = "Apache Arrow")
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ x2 = substr(x1, 1, 6), # Apache
+ x3 = substr(x1, 8, 12) # Arrow
+ ) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ x2 = substring(x1, 1, 6), # Apache
+ x3 = substring(x1, 8, 12) # Arrow
+ ) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ x2 = str_sub(x1, 1, 6), # Apache
+ x3 = str_sub(x1, 8, 12) # Arrow
+ ) %>%
+ collect(),
+ df
+ )
})
Review comment:
The tests are now way shorter than they were before and don't include any of those awkward test cases like negative numbers that we were discussing on the ticket - maybe we should add them back in?
##########
File path: r/src/compute.cpp
##########
@@ -316,6 +316,19 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
return std::make_shared<Options>(max_splits, reverse);
}
+ if (func_name == "utf8_slice_codeunits") {
+ using Options = arrow::compute::SliceOptions;
+ int64_t start = 1;
+ int64_t stop = -1;
Review comment:
See the final comment here for more info but happy to explain if that doesn't make sense: https://issues.apache.org/jira/browse/ARROW-13259
--
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
[GitHub] [arrow] thisisnic commented on pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#issuecomment-880694775
@nealrichardson - ready for re-review, thanks!
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r670254295
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +284,81 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(stop) == 1,
+ msg = "`stop` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start <= 0) {
+ start <- 1
+ }
+
+ if (stop < start) {
+ stop <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ string,
+ options = list(start = start - 1L, stop = stop)
+ )
+}
+
+nse_funcs$substring <- function(text, first, last = 1000000L) {
+ assert_that(
+ length(first) == 1,
+ msg = "`first` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(last) == 1,
+ msg = "`last` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (first <= 0) {
+ first <- 1
+ }
+
+ if (last < first) {
+ last <- 0
+ }
+
+ Expression$create(
+ "utf8_slice_codeunits",
+ text,
+ options = list(start = first - 1L, stop = last)
+ )
+}
+
+nse_funcs$str_sub <- function(string, start = 1L, end = -1L) {
+ assert_that(
+ length(start) == 1,
+ msg = "`start` must be length 1 - other lengths are not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "`end` must be length 1 - other lengths are not supported in Arrow"
+ )
+
+ if (start == 0) start <- 1
+
+ if (end == -1) end <- .Machine$integer.max
+
+ if (end < start) end <- 0
+
+ if (start > 0) start <- start - 1L
Review comment:
The subtracting of 1 is done in this code block in this function, because it's conditional on `start` being greater than 0.
The other versions don't allow using negative values to count from the end backwards, so while in the others start <= 0 isn't valid, here it is.
We only subtract 1 from start when start is > 0 because:
* we normally need to subtract 1 from `start` because C++ is 0-based and R is 1-based so they're counting from different points
* we don't need to subtract 1 from `end` as R counts inclusively (i.e. returned string includes the item at position `end`) whereas C++ counts exclusively (i.e. returned string includes *up to* the item at position `end` which effectively cancels out the difference due to indexing
* `str_sub` treats a `start` value of 0 or 1 as the same thing, which is why here, the subtraction is not done when `start` == 0 (and so resulting in them both passing a `start` value of 0 being passed to `utf8_slice_codeunits`)
* a `start` value < 0 is valid as both `str_sub` and `utf8_slice_codeunits` can count backwards from the end with -1 being the final character in the string, -2 being the second to last character, etc.
I'll add some of this to the code in the form of a comment
--
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
[GitHub] [arrow] pachadotdev commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
pachadotdev commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r668242367
##########
File path: r/src/compute.cpp
##########
@@ -316,6 +316,19 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
return std::make_shared<Options>(max_splits, reverse);
}
+ if (func_name == "utf8_slice_codeunits") {
+ using Options = arrow::compute::SliceOptions;
+ int64_t start = 1;
+ int64_t stop = -1;
Review comment:
ready !! i added new tests and I commented the expected results
--
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
[GitHub] [arrow] nealrichardson closed pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #10624:
URL: https://github.com/apache/arrow/pull/10624
--
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
[GitHub] [arrow] pachadotdev commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
pachadotdev commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r667533252
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +280,45 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "Start of length != 1 not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "End of length != 1 not supported in Arrow"
+ )
+
+ if (start > stop) {
+ start <- 0
+ stop <- 0
+ } else {
+ start <- max(0, start - 1)
+ stop <- max(0, stop)
+ start_stop <- c(min(start, stop), max(start, stop))
+ start <- start_stop[1]
+ stop <- start_stop[2]
+ }
Review comment:
hmmm.... maybe it's just a bad try from me to avoid situations where start > end
i'l lremove 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
[GitHub] [arrow] thisisnic commented on pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#issuecomment-880129596
@nealrichardson - have made some updates; please could you re-review this when you have a chance? Tomorrow I'm going to add in the tests for warnings/errors raised when incorrect parameter values are supplied but is there anything else that needs updating in this PR?
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r664040870
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substring(foo, 1, 6), # Apache
+ bar2 = substring(foo, 0, 6), # Apache
+ bar3 = substring(foo, -1, 6), # Apache
+ bar4 = substring(foo, 6, 1), # ""
+ bar5 = substring(foo, -1, -2), # ""
+ bar6 = substring(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
+ collect(),
+ df
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = str_sub(foo, 1, 6), # Apache
+ bar2 = str_sub(foo, 0, 6), # Apache
+ bar3 = str_sub(foo, -1, 6), # Apache
+ bar4 = str_sub(foo, 6, 1), # ""
+ bar5 = str_sub(foo, -1, -2), # ""
+ bar6 = str_sub(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
+ collect(),
+ df
+ )
+})
Review comment:
```suggestion
test_that("substrings", {
df <- tibble(
x = "Apache Arrow"
)
# substr
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 0, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, -1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 6, 1)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, -1, -2)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, 8, 12)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substr(x, -5, -1)) %>%
collect(),
df
)
# substring
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 0, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, -1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 6, 1)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, -1, -2)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, 8, 12)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = substring(x, -5, -1)) %>%
collect(),
df
)
# str_sub
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 0, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, -1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 6, 1)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, -1, -2)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 1, 6)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, 8, 12)) %>%
collect(),
df
)
expect_dplyr_equal(
input %>%
mutate(y = str_sub(x, -5, -1)) %>%
collect(),
df
)
})
```
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r662899545
##########
File path: r/R/expression.R
##########
@@ -32,6 +32,8 @@
"str_to_upper" = "utf8_upper",
"str_reverse" = "utf8_reverse",
# str_trim is defined in dplyr-functions.R
+ # str_sub is defined in dplyr-functions.R
+ # substr is defined in dplyr-functions.R
"year" = "year",
Review comment:
Should we mention `substring` here as well?
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
Review comment:
No need to set up string "Apache Arrow" via variable `foo` as it's already been set up as variable `x`.
```suggestion
bar1 = substr(x, 1, 6), # Apache
bar2 = substr(x, 0, 6), # Apache
bar3 = substr(x, -1, 6), # Apache
bar4 = substr(x, 6, 1), # ""
bar5 = substr(x, -1, -2), # ""
bar6 = substr(x, 8, 12) # Arrow
```
##########
File path: r/R/dplyr-functions.R
##########
@@ -280,6 +280,45 @@ nse_funcs$str_trim <- function(string, side = c("both", "left", "right")) {
Expression$create(trim_fun, string)
}
+nse_funcs$substr <- function(string, start, stop) {
+ assert_that(
+ length(start) == 1,
+ msg = "Start of length != 1 not supported in Arrow"
+ )
+ assert_that(
+ length(end) == 1,
+ msg = "End of length != 1 not supported in Arrow"
+ )
+
+ if (start > stop) {
+ start <- 0
+ stop <- 0
+ } else {
+ start <- max(0, start - 1)
+ stop <- max(0, stop)
+ start_stop <- c(min(start, stop), max(start, stop))
+ start <- start_stop[1]
+ stop <- start_stop[2]
+ }
Review comment:
I'm a little confused here - please can you give me an example of values that could be supplied as `start` and `stop` which would make it necessary to use `min()` and `max()` here to put the values back in the correct order?
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
Review comment:
Please can you split these out into separate tests? It makes it easier to follow and fix issues.
##########
File path: r/tests/testthat/test-dplyr-string-functions.R
##########
@@ -866,3 +866,57 @@ test_that("str_like", {
df
)
})
+
+test_that("substrings", {
+ df <- tibble(
+ x = "Apache Arrow"
+ )
+
+ expect_dplyr_equal(
+ input %>%
+ mutate(
+ foo = "Apache Arrow",
+ bar1 = substr(foo, 1, 6), # Apache
+ bar2 = substr(foo, 0, 6), # Apache
+ bar3 = substr(foo, -1, 6), # Apache
+ bar4 = substr(foo, 6, 1), # ""
+ bar5 = substr(foo, -1, -2), # ""
+ bar6 = substr(foo, 8, 12) # Arrow
+ ) %>%
+ select(bar1:bar6) %>%
Review comment:
```suggestion
```
--
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
[GitHub] [arrow] pachadotdev commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
pachadotdev commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r668226798
##########
File path: r/src/compute.cpp
##########
@@ -316,6 +316,19 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
return std::make_shared<Options>(max_splits, reverse);
}
+ if (func_name == "utf8_slice_codeunits") {
+ using Options = arrow::compute::SliceOptions;
+ int64_t start = 1;
+ int64_t stop = -1;
Review comment:
yes !!!!
at the moment, I have added the additional "+1" to the R functions
if the backend changes, Iĺl adapt the functions
--
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
[GitHub] [arrow] thisisnic commented on a change in pull request #10624: ARROW-12992: [R] bindings for substr(), substring(), str_sub()
Posted by GitBox <gi...@apache.org>.
thisisnic commented on a change in pull request #10624:
URL: https://github.com/apache/arrow/pull/10624#discussion_r669142885
##########
File path: r/src/compute.cpp
##########
@@ -316,6 +316,19 @@ std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(
return std::make_shared<Options>(max_splits, reverse);
}
+ if (func_name == "utf8_slice_codeunits") {
+ using Options = arrow::compute::SliceOptions;
+ int64_t start = 1;
+ int64_t stop = -1;
Review comment:
Sorry, I didn't explain this properly, my fault! What I mean is that here stop has been set to `-1` but the default C++ value isn't `-1`, so the default value here probably shouldn't be `-1` either.
Check out this line of code here that shows the default value of `stop` in the C++: https://github.com/apache/arrow/blob/7114c4b6fdb63 9b3500d77cfd66649af8c5c5e6b/cpp/src/arrow/compute/api_scalar.h#L205-L206
The default value of `stop` is `std::numeric_limits<int64_t>::max()` which is the biggest int64. So if you don't supply a value for `stop` and just use the default, this allows you to slice to the end of the string.
In some of the other functions in this file, this line has been used to set the default values to the ones from the C++ code:
`std::make_shared<Options>(Options::Defaults());`
Maybe instead of manually setting the value of stop to `-1`, if you use the line above, it might make it easier to fix some of the failing tests as now you'll be able to slice to the end of a string. If this doesn't make sense, let me know!
--
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