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 2022/02/02 20:14:26 UTC
[GitHub] [arrow] paleolimbot opened a new pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
paleolimbot opened a new pull request #12324:
URL: https://github.com/apache/arrow/pull/12324
This PR adds `concat_arrays()`, which is available in the Python bindings but not available in the R bindings. It's a difficult operation to replicate (although can be done using `ChunkedArray$Take()`) and is a fairly straightforward binding. I did a tiny bit more than the Python implementation to support a common type (like `Array$create()`) because it was easy...it sounds like a more robust approach is coming (via ARROW-14705 or by exposing `MergeOptions` to `arrow::Concatenate()`).
Reprex:
``` r
library(arrow, warn.conflicts = FALSE)
concat_arrays()
#> Array
#> <null>
#> 0 nulls
concat_arrays(type = int64())
#> Array
#> <int64>
#> []
concat_arrays(Array$create(1:3), Array$create(4:5))
#> Array
#> <int32>
#> [
#> 1,
#> 2,
#> 3,
#> 4,
#> 5
#> ]
# limited support for concatenating Arrays of differing types since this
# isn't exposed by arrow::Concatenate() yet
concat_arrays(
Array$create(1:3, type = float32()),
Array$create(4:5, type = float64()),
type = int64()
)
#> Array
#> <int64>
#> [
#> 1,
#> 2,
#> 3,
#> 4,
#> 5
#> ]
concat_arrays(
Array$create(1:3, type = float32()),
Array$create(4:5, type = float64())
)
#> Error: Invalid: arrays to be concatenated must be identically typed, but float and double were encountered.
```
--
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] jonkeane commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806267955
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
+ expect_r6_class(c(Array$create(1L), Array$create(1L)), "Array")
+
+ struct <- call_function(
+ "make_struct",
+ Array$create(1L),
+ options = list(field_names = "")
+ )
+ expect_r6_class(c(struct, struct), "StructArray")
+
+ list <- Array$create(list(1))
+ expect_r6_class(c(list, list), "ListArray")
+
+ list <- Array$create(list(), type = large_list_of(float64()))
+ expect_r6_class(c(list, list), "LargeListArray")
+
+ list <- Array$create(list(),type = fixed_size_list_of(float64(), 1L))
+ expect_r6_class(c(list, list), "FixedSizeListArray")
+
+ list <- Array$create(list(),type = map_of(string(), float64()))
Review comment:
```suggestion
list <- Array$create(list(), type = map_of(string(), float64()))
```
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
+ expect_r6_class(c(Array$create(1L), Array$create(1L)), "Array")
+
+ struct <- call_function(
+ "make_struct",
+ Array$create(1L),
+ options = list(field_names = "")
+ )
+ expect_r6_class(c(struct, struct), "StructArray")
+
+ list <- Array$create(list(1))
+ expect_r6_class(c(list, list), "ListArray")
+
+ list <- Array$create(list(), type = large_list_of(float64()))
+ expect_r6_class(c(list, list), "LargeListArray")
+
+ list <- Array$create(list(),type = fixed_size_list_of(float64(), 1L))
Review comment:
```suggestion
list <- Array$create(list(), type = fixed_size_list_of(float64(), 1L))
```
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
Review comment:
Could? Should? you also test for things like `c(Array$create(1L), 2L)` I imagine that would error, but does it with an informative message?
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Out of (morbid?) curiosity: what does it do without the subclasses?
--
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] jonkeane closed pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane closed pull request #12324:
URL: https://github.com/apache/arrow/pull/12324
--
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] paleolimbot commented on pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1031494704
That's a great point! `c()` is a little funky but I added a method that brings it more in line with what a user would expect than the default behaviour that you demoed above:
``` r
# remotes::install_github("apache/arrow/r#12324")
library(arrow, warn.conflicts = FALSE)
x <- Array$create(c(1, 2, 3))
y <- Array$create(c(4, 5, 6))
c(x, y)
#> Array
#> <double>
#> [
#> 1,
#> 2,
#> 3,
#> 4,
#> 5,
#> 6
#> ]
```
<sup>Created on 2022-02-07 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
--
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] ursabot edited a comment on pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1040680517
Benchmark runs are scheduled for baseline = 6a2ee11d30676f99e40dfd9af94915981180510b and contender = cca3800bd95c4476f695cccf2bf9f39abacf4bf3. cca3800bd95c4476f695cccf2bf9f39abacf4bf3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/76f91706c7c7454abc183876a93df010...56547056e4004c21a8e0a426e97d5187/)
[Finished :arrow_down:0.04% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e04b89e7418f407e9273c377eb810a07...3fe240ea54c542d8918963d4704e702d/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3ed781cf1b704a5eb320d8509aa27f51...6b3eb5d148714c0e86ce9d85df3558d4/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ed5b648699064c1ca4949d72bed2ac83...d19cb70b0fde4c21bc02b1120755ded2/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r807142173
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Done!
--
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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r807192866
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,75 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
Review comment:
In R, `vctrs::vec_c()` returns `NULL`, which can be inserted into another `vec_c()` call without affecting the (type or size of the) result. A `null()` of length 0 works similarly in Arrow (it can be cast to any type). It's nice when programming (for the same reason that `all(logical(0))` is `TRUE`).
``` r
library(arrow, warn.conflicts = FALSE)
library(vctrs, warn.conflicts = FALSE)
some_user_input1 <- list()
some_user_input2 <- list()
result_of_some_op <- concat_arrays(!!! some_user_input1, !!! some_user_input2)
some_user_input <- list(1L, 2:4)
(final_result <- concat_arrays(!!! some_user_input, result_of_some_op, type = int32()))
#> Array
#> <int32>
#> [
#> 1,
#> 2,
#> 3,
#> 4
#> ]
```
--
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] jonkeane commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806267955
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
+ expect_r6_class(c(Array$create(1L), Array$create(1L)), "Array")
+
+ struct <- call_function(
+ "make_struct",
+ Array$create(1L),
+ options = list(field_names = "")
+ )
+ expect_r6_class(c(struct, struct), "StructArray")
+
+ list <- Array$create(list(1))
+ expect_r6_class(c(list, list), "ListArray")
+
+ list <- Array$create(list(), type = large_list_of(float64()))
+ expect_r6_class(c(list, list), "LargeListArray")
+
+ list <- Array$create(list(),type = fixed_size_list_of(float64(), 1L))
+ expect_r6_class(c(list, list), "FixedSizeListArray")
+
+ list <- Array$create(list(),type = map_of(string(), float64()))
Review comment:
```suggestion
list <- Array$create(list(), type = map_of(string(), float64()))
```
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
+ expect_r6_class(c(Array$create(1L), Array$create(1L)), "Array")
+
+ struct <- call_function(
+ "make_struct",
+ Array$create(1L),
+ options = list(field_names = "")
+ )
+ expect_r6_class(c(struct, struct), "StructArray")
+
+ list <- Array$create(list(1))
+ expect_r6_class(c(list, list), "ListArray")
+
+ list <- Array$create(list(), type = large_list_of(float64()))
+ expect_r6_class(c(list, list), "LargeListArray")
+
+ list <- Array$create(list(),type = fixed_size_list_of(float64(), 1L))
Review comment:
```suggestion
list <- Array$create(list(), type = fixed_size_list_of(float64(), 1L))
```
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
Review comment:
Could? Should? you also test for things like `c(Array$create(1L), 2L)` I imagine that would error, but does it with an informative message?
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Out of (morbid?) curiosity: what does it do without the subclasses?
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
_nods_ hmmmm yeah that is madness! And if we do define those extra methods that were here before, does this madness go away? (e.g. if we do `c( wk::xy(1:2, 1:2), Array$create(1:3))` what happens?)
If we still get weird errors there, I wonder if exposing `c()` is all that helpful before we do it the "right way"?
We'll export our own concatenate method — so it's possible to use it, but I suspect that `c()` working only when the first element is an array will be a less-than-fun experience (with not too much upside anyway!).
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,65 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("concat_arrays() coerces its input to Array", {
+ concat_ints <- concat_arrays(1L, 2L)
+ expect_true(concat_ints$type == int32())
+ expect_true(all(concat_ints == Array$create(c(1L, 2L))))
Review comment:
Could we also add some non-sensical casts here too? (or at least what I would assume to be nonsensical!
`concat_arrays(Array$create(1L), "foo")` or the like?
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Hmm, maybe this is idiosyncratic but stuffing the arrays themselves into a vector seems not quite as bad to me, but I can see how that's (probably) not what people want.
Then again, even without defining them we get odd behaviors depending on the order already anyway. This is from an arrow install that is not on this branch:
```
> c( wk::xy(1:2, 1:2), Array$create(1:3))
Error: Can't combine 'wk_rcrd' objects that do not have identical classes.
> c(Array$create(1:3), wk::xy(1:2, 1:2))
[[1]]
Array
<int32>
[
1,
2,
3
]
$x
[1] 1 2
$y
[1] 1 2
```
I'm fine keeping it, either way we should mention in our docs that `c()` will probably be surprising
##########
File path: r/R/array.R
##########
@@ -216,6 +216,49 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' Concatenates zero or more [Array] objects into a single
+#' array. This operation will copy its input; if you need
+#' the behavior of a single Array but don't need a
Review comment:
```suggestion
#' array. This operation will make a copy of its input; if you need
#' the behavior of a single Array but don't need a
```
this is minor, but I think makes it a tiny bit clearer that it's doing a copying action (we might even go stronger?)
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,65 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("concat_arrays() coerces its input to Array", {
+ concat_ints <- concat_arrays(1L, 2L)
+ expect_true(concat_ints$type == int32())
+ expect_true(all(concat_ints == Array$create(c(1L, 2L))))
Review comment:
🎉
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,75 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
Review comment:
Thanks for testing this! It is a bit funny, but I'm sure there's some convoluted reason to need to do 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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806888074
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,65 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("concat_arrays() coerces its input to Array", {
+ concat_ints <- concat_arrays(1L, 2L)
+ expect_true(concat_ints$type == int32())
+ expect_true(all(concat_ints == Array$create(c(1L, 2L))))
Review comment:
Done!
--
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] jonkeane commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r807207163
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,75 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
Review comment:
Aaah 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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806877934
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
As @wjones127 noted above, if we *don't* define it we get even weirder behaviour. I think it's more intuitive to define it than omit but am happy to leave it out too. The upside is that it's the first thing an R user might think to do (as Will thought to do immediately based on the comment above).
--
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] ursabot edited a comment on pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1040680517
Benchmark runs are scheduled for baseline = 6a2ee11d30676f99e40dfd9af94915981180510b and contender = cca3800bd95c4476f695cccf2bf9f39abacf4bf3. cca3800bd95c4476f695cccf2bf9f39abacf4bf3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/76f91706c7c7454abc183876a93df010...56547056e4004c21a8e0a426e97d5187/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e04b89e7418f407e9273c377eb810a07...3fe240ea54c542d8918963d4704e702d/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3ed781cf1b704a5eb320d8509aa27f51...6b3eb5d148714c0e86ce9d85df3558d4/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ed5b648699064c1ca4949d72bed2ac83...d19cb70b0fde4c21bc02b1120755ded2/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806789891
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Good call! I thought this would fail:
``` r
library(arrow, warn.conflicts = FALSE)
arr1 <- Array$create(1:3)
arr2 <- Array$create(4:5)
c(arr1, arr2)
#> Array
#> <int32>
#> [
#> 1,
#> 2,
#> 3,
#> 4,
#> 5
#> ]
class(arr1) <- c("SomeSubclass", class(arr1))
class(arr2) <- c("SomeOtherSubclass", class(arr2))
c(arr1, arr2)
#> Array
#> <int32>
#> [
#> 1,
#> 2,
#> 3,
#> 4,
#> 5
#> ]
```
...but it didn't, and removing all the extra `c()` methods didn't cause any of the tests to break, which means that for all practical purposes we don't need them. A better example of the madness I was thinking of is this:
``` r
library(arrow, warn.conflicts = FALSE)
c(Array$create(1:3), wk::xy(1:2, 1:2))
#> Error: Invalid: arrays to be concatenated must be identically typed, but int32 and list<item: double> were encountered.
c( wk::xy(1:2, 1:2), Array$create(1:3))
#> Error: Can't combine 'wk_rcrd' objects that do not have identical classes.
```
(where if more than one `c()` method is defined you'll get different results depending on which one is first).
The "right way" to do this is a full vctrs implementation but I don't think it's time for that quite yet.
--
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] ursabot commented on pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1040680517
Benchmark runs are scheduled for baseline = 6a2ee11d30676f99e40dfd9af94915981180510b and contender = cca3800bd95c4476f695cccf2bf9f39abacf4bf3. cca3800bd95c4476f695cccf2bf9f39abacf4bf3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/76f91706c7c7454abc183876a93df010...56547056e4004c21a8e0a426e97d5187/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e04b89e7418f407e9273c377eb810a07...3fe240ea54c542d8918963d4704e702d/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3ed781cf1b704a5eb320d8509aa27f51...6b3eb5d148714c0e86ce9d85df3558d4/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ed5b648699064c1ca4949d72bed2ac83...d19cb70b0fde4c21bc02b1120755ded2/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806791553
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
Review comment:
Good call! This is another example of the madness around `c()` (which we can do little about except circumvent it by eventually defining a vctrs implementation).
``` r
library(arrow, warn.conflicts = FALSE)
c(Array$create(1L), 2L)
#> Array
#> <int32>
#> [
#> 1,
#> 2
#> ]
c(2L, Array$create(1L))
#> [[1]]
#> [1] 2
#>
#> [[2]]
#> Array
#> <int32>
#> [
#> 1
#> ]
```
<sup>Created on 2022-02-15 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
--
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] jonkeane commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806894046
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Hmm, maybe this is idiosyncratic but stuffing the arrays themselves into a vector seems not quite as bad to me, but I can see how that's (probably) not what people want.
Then again, even without defining them we get odd behaviors depending on the order already anyway. This is from an arrow install that is not on this branch:
```
> c( wk::xy(1:2, 1:2), Array$create(1:3))
Error: Can't combine 'wk_rcrd' objects that do not have identical classes.
> c(Array$create(1:3), wk::xy(1:2, 1:2))
[[1]]
Array
<int32>
[
1,
2,
3
]
$x
[1] 1 2
$y
[1] 1 2
```
I'm fine keeping it, either way we should mention in our docs that `c()` will probably be surprising
--
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] ursabot edited a comment on pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1040680517
Benchmark runs are scheduled for baseline = 6a2ee11d30676f99e40dfd9af94915981180510b and contender = cca3800bd95c4476f695cccf2bf9f39abacf4bf3. cca3800bd95c4476f695cccf2bf9f39abacf4bf3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/76f91706c7c7454abc183876a93df010...56547056e4004c21a8e0a426e97d5187/)
[Scheduled] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e04b89e7418f407e9273c377eb810a07...3fe240ea54c542d8918963d4704e702d/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3ed781cf1b704a5eb320d8509aa27f51...6b3eb5d148714c0e86ce9d85df3558d4/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ed5b648699064c1ca4949d72bed2ac83...d19cb70b0fde4c21bc02b1120755ded2/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806789891
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Good call! I thought this would fail:
``` r
library(arrow, warn.conflicts = FALSE)
arr1 <- Array$create(1:3)
arr2 <- Array$create(4:5)
c(arr1, arr2)
#> Array
#> <int32>
#> [
#> 1,
#> 2,
#> 3,
#> 4,
#> 5
#> ]
class(arr1) <- c("SomeSubclass", class(arr1))
class(arr2) <- c("SomeOtherSubclass", class(arr2))
c(arr1, arr2)
#> Array
#> <int32>
#> [
#> 1,
#> 2,
#> 3,
#> 4,
#> 5
#> ]
```
...but it didn't, and removing all the extra `c()` methods didn't cause any of the tests to break, which means that for all practical purposes we don't need them. A better example of the madness I was thinking of is this:
``` r
library(arrow, warn.conflicts = FALSE)
c(Array$create(1:3), wk::xy(1:2, 1:2))
#> Error: Invalid: arrays to be concatenated must be identically typed, but int32 and list<item: double> were encountered.
c( wk::xy(1:2, 1:2), Array$create(1:3))
#> Error: Can't combine 'wk_rcrd' objects that do not have identical classes.
```
(where if more than one `c()` method is defined you'll get different results depending on which one is first).
The "right way" to do this is a full vctrs implementation but I don't think it's time for that quite yet.
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
Review comment:
Good call! This is another example of the madness around `c()` (which we can do little about except circumvent it by eventually defining a vctrs implementation).
``` r
library(arrow, warn.conflicts = FALSE)
c(Array$create(1L), 2L)
#> Array
#> <int32>
#> [
#> 1,
#> 2
#> ]
c(2L, Array$create(1L))
#> [[1]]
#> [1] 2
#>
#> [[2]]
#> Array
#> <int32>
#> [
#> 1
#> ]
```
<sup>Created on 2022-02-15 by the [reprex package](https://reprex.tidyverse.org) (v2.0.1)</sup>
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
Review comment:
...I added a test from the `concat_arrays()` end since we do have control over that!
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
As @wjones127 noted above, if we *don't* define it we get even weirder behaviour. I think it's more intuitive to define it than omit but am happy to leave it out too. The upside is that it's the first thing an R user might think to do (as Will thought to do immediately based on the comment above).
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,65 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("concat_arrays() coerces its input to Array", {
+ concat_ints <- concat_arrays(1L, 2L)
+ expect_true(concat_ints$type == int32())
+ expect_true(all(concat_ints == Array$create(c(1L, 2L))))
Review comment:
Done!
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
Done!
--
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] wjones127 commented on pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
wjones127 commented on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1028371974
Current behavior of the basic concat function `c` is this:
```r
> x <- Array$create(c(1, 2, 3))
> y <- Array$create(c(4, 5, 6))
> c(x, y)
[[1]]
Array
<double>
[
1,
2,
3
]
[[2]]
Array
<double>
[
4,
5,
6
]
```
Could we make it so it uses this `concat_arrays` logic instead? Is that even possible?
--
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] ursabot edited a comment on pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1040680517
Benchmark runs are scheduled for baseline = 6a2ee11d30676f99e40dfd9af94915981180510b and contender = cca3800bd95c4476f695cccf2bf9f39abacf4bf3. cca3800bd95c4476f695cccf2bf9f39abacf4bf3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/76f91706c7c7454abc183876a93df010...56547056e4004c21a8e0a426e97d5187/)
[Finished :arrow_down:0.04% :arrow_up:0.04%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/e04b89e7418f407e9273c377eb810a07...3fe240ea54c542d8918963d4704e702d/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/3ed781cf1b704a5eb320d8509aa27f51...6b3eb5d148714c0e86ce9d85df3558d4/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ed5b648699064c1ca4949d72bed2ac83...d19cb70b0fde4c21bc02b1120755ded2/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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 #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#issuecomment-1028319062
--
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] jonkeane commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r807163224
##########
File path: r/R/array.R
##########
@@ -216,6 +216,49 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' Concatenates zero or more [Array] objects into a single
+#' array. This operation will copy its input; if you need
+#' the behavior of a single Array but don't need a
Review comment:
```suggestion
#' array. This operation will make a copy of its input; if you need
#' the behavior of a single Array but don't need a
```
this is minor, but I think makes it a tiny bit clearer that it's doing a copying action (we might even go stronger?)
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,65 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("concat_arrays() coerces its input to Array", {
+ concat_ints <- concat_arrays(1L, 2L)
+ expect_true(concat_ints$type == int32())
+ expect_true(all(concat_ints == Array$create(c(1L, 2L))))
Review comment:
🎉
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,75 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
Review comment:
Thanks for testing this! It is a bit funny, but I'm sure there's some convoluted reason to need to do 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] paleolimbot commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806798917
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,59 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("c() works for Array", {
Review comment:
...I added a test from the `concat_arrays()` end since we do have control over that!
--
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] jonkeane commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806871651
##########
File path: r/R/array.R
##########
@@ -216,6 +216,76 @@ Array$create <- function(x, type = NULL) {
#' @include arrowExports.R
Array$import_from_c <- ImportArray
+
+#' Concatenate zero or more Arrays
+#'
+#' @param ... zero or more [Array] objects to concatenate
+#' @param type An optional `type` describing the desired
+#' type for the final Array.
+#'
+#' @return An [Array]
+#' @export
+#'
+#' @examples
+#' concat_arrays(Array$create(1:3), Array$create(4:5))
+#'
+concat_arrays <- function(..., type = NULL) {
+ dots <- lapply(list2(...), Array$create, type = type)
+
+ if (length(dots) == 0 && is.null(type)) {
+ return(Array$create(logical(), type = null()))
+ } else if (length(dots) == 0) {
+ return(Array$create(logical(), type = null())$cast(type))
+ }
+
+ if (!is.null(type)) {
+ dots <- lapply(dots, function(array) array$cast(type))
+ }
+
+ arrow__Concatenate(dots)
+}
+
+# The c() method uses non-standard dispatch in R
+# and has some peculiarities when multiple types are passed to ....
+# However, with a method defined for all subclasses of Array, it will
+# do what a user expects most of the time.
Review comment:
_nods_ hmmmm yeah that is madness! And if we do define those extra methods that were here before, does this madness go away? (e.g. if we do `c( wk::xy(1:2, 1:2), Array$create(1:3))` what happens?)
If we still get weird errors there, I wonder if exposing `c()` is all that helpful before we do it the "right way"?
We'll export our own concatenate method — so it's possible to use it, but I suspect that `c()` working only when the first element is an array will be a less-than-fun experience (with not too much upside anyway!).
--
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] jonkeane commented on a change in pull request #12324: ARROW-15013: [R] Expose concatenate at the R level
Posted by GitBox <gi...@apache.org>.
jonkeane commented on a change in pull request #12324:
URL: https://github.com/apache/arrow/pull/12324#discussion_r806873278
##########
File path: r/tests/testthat/test-Array.R
##########
@@ -989,6 +989,65 @@ test_that("auto int64 conversion to int can be disabled (ARROW-10093)", {
})
})
+test_that("concat_arrays works", {
+ concat_empty <- concat_arrays()
+ expect_true(concat_empty$type == null())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_empty_typed <- concat_arrays(type = int64())
+ expect_true(concat_empty_typed$type == int64())
+ expect_equal(concat_empty$length(), 0L)
+
+ concat_int <- concat_arrays(Array$create(1:3), Array$create(4:5))
+ expect_true(concat_int$type == int32())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ concat_int64 <- concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64()),
+ type = int64()
+ )
+ expect_true(concat_int64$type == int64())
+ expect_true(all(concat_int == Array$create(1:5)))
+
+ expect_error(
+ concat_arrays(
+ Array$create(1:3),
+ Array$create(4:5, type = int64())
+ ),
+ "must be identically typed"
+ )
+})
+
+test_that("concat_arrays() coerces its input to Array", {
+ concat_ints <- concat_arrays(1L, 2L)
+ expect_true(concat_ints$type == int32())
+ expect_true(all(concat_ints == Array$create(c(1L, 2L))))
Review comment:
Could we also add some non-sensical casts here too? (or at least what I would assume to be nonsensical!
`concat_arrays(Array$create(1L), "foo")` or the like?
--
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