You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/08/17 19:56:29 UTC
[GitHub] [arrow-nanoarrow] paleolimbot opened a new pull request, #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
paleolimbot opened a new pull request, #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279
When collecting an array stream with unknown size into a data.frame, nanoarrow has pretty terrible performance. This is because it collects and converts all batches and does `c()` or `rbind()` on the result. This is particularly bad when collecting many tiny batches (e.g., like those returned by many ADBC drivers).
`convert_array_stream()` has long had a "preallocate + fill" mode when `size` was explicitly set. Recently, the addition of `basic_array_stream()` makes it possible to recreate an array stream from a previously-collected result. Collectively, this means we can collect the whole stream, compute the size, and then call `convert_array_stream()` with a known size.
Before this PR:
``` r
library(nanoarrow)
data_frames <- replicate(
1000,
nanoarrow:::vec_gen(
data.frame(x = logical(), y = double(), z = character()),
n = 1000
),
simplify = FALSE
)
bench::mark(
convert_known_size = {
stream <- basic_array_stream(data_frames, validate = FALSE)
convert_array_stream(stream, size = 1000 * 1000)
},
convert_unknown_size = {
stream <- basic_array_stream(data_frames, validate = FALSE)
as.data.frame(stream)
},
convert_arrow_altrep = {
options(arrow.use_altrep = TRUE)
stream <- basic_array_stream(data_frames, validate = FALSE)
reader <- arrow::as_record_batch_reader(stream)
as.data.frame(as.data.frame(reader))
},
convert_arrow = {
options(arrow.use_altrep = FALSE)
stream <- basic_array_stream(data_frames, validate = FALSE)
reader <- arrow::as_record_batch_reader(stream)
as.data.frame(as.data.frame(reader))
},
min_iterations = 20
)
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 4 × 6
#> expression min median `itr/sec` mem_alloc `gc/sec`
#> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl>
#> 1 convert_known_size 196.9ms 234ms 3.85 23.1MB 4.23
#> 2 convert_unknown_size 375.8ms 479ms 2.12 429.3MB 13.2
#> 3 convert_arrow_altrep 67.4ms 164ms 4.78 20.4MB 6.70
#> 4 convert_arrow 107.8ms 240ms 2.96 22.9MB 3.56
```
After this PR:
``` r
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 4 × 6
#> expression min median `itr/sec` mem_alloc `gc/sec`
#> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl>
#> 1 convert_known_size 203.4ms 225ms 3.99 23.2MB 3.99
#> 2 convert_unknown_size 266.5ms 396ms 0.895 23.1MB 2.60
#> 3 convert_arrow_altrep 68.5ms 214ms 3.76 20.4MB 3.76
#> 4 convert_arrow 130.6ms 227ms 2.93 22.9MB 3.23
```
<sup>Created on 2023-08-17 with [reprex v2.0.2](https://reprex.tidyverse.org)</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-nanoarrow] paleolimbot merged pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279
--
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-nanoarrow] paleolimbot commented on a diff in pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279#discussion_r1300212953
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
+ basic_stream <- .Call(nanoarrow_c_basic_array_stream, batches, schema, FALSE)
+ convert_array_stream(basic_stream, to = to, size = sum(lengths))
+}
+
+#' @rdname convert_array_stream
+#' @export
+collect_array_stream <- function(array_stream, n = Inf, schema = NULL,
+ validate = TRUE) {
+ stopifnot(
+ inherits(array_stream, "nanoarrow_array_stream")
+ )
+
+ if (is.null(schema)) {
+ schema <- .Call(nanoarrow_c_array_stream_get_schema, array_stream)
+ }
+
batches <- vector("list", 1024L)
n_batches <- 0L
get_next <- array_stream$get_next
- while (!is.null(array <- get_next(schema, validate = FALSE)) && (n_batches < n)) {
+ while (!is.null(array <- get_next(schema, validate = validate)) && (n_batches < n)) {
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-nanoarrow] krlmlr commented on a diff in pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "krlmlr (via GitHub)" <gi...@apache.org>.
krlmlr commented on code in PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279#discussion_r1299384346
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
+ basic_stream <- .Call(nanoarrow_c_basic_array_stream, batches, schema, FALSE)
+ convert_array_stream(basic_stream, to = to, size = sum(lengths))
+}
+
+#' @rdname convert_array_stream
+#' @export
+collect_array_stream <- function(array_stream, n = Inf, schema = NULL,
+ validate = TRUE) {
+ stopifnot(
+ inherits(array_stream, "nanoarrow_array_stream")
+ )
+
+ if (is.null(schema)) {
+ schema <- .Call(nanoarrow_c_array_stream_get_schema, array_stream)
+ }
+
batches <- vector("list", 1024L)
n_batches <- 0L
get_next <- array_stream$get_next
- while (!is.null(array <- get_next(schema, validate = FALSE)) && (n_batches < n)) {
+ while (!is.null(array <- get_next(schema, validate = validate)) && (n_batches < n)) {
n_batches <- n_batches + 1L
- batches[[n_batches]] <- .Call(nanoarrow_c_convert_array, array, to)
+ batches[[n_batches]] <- array
Review Comment:
Without array doubling, the run time still might be superlinear for very many batches. I have no idea if this is of practical relevance, though.
``` r
fill_list <- function(n) {
out <- vector("list", 1024)
for (i in seq_len(n)) {
out[[i]] <- i
}
NULL
}
bench::mark(fill_list(1e6), fill_list(2e6), fill_list(4e6), fill_list(8e6))
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 4 × 6
#> expression min median `itr/sec` mem_alloc `gc/sec`
#> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl>
#> 1 fill_list(1e+06) 446.17ms 453.62ms 2.20 165.33MB 2.20
#> 2 fill_list(2e+06) 1.38s 1.38s 0.723 334.95MB 2.17
#> 3 fill_list(4e+06) 4.72s 4.72s 0.212 663.35MB 1.69
#> 4 fill_list(8e+06) 8.42s 8.42s 0.119 1.28GB 1.42
```
<sup>Created on 2023-08-20 with [reprex v2.0.2](https://reprex.tidyverse.org)</sup>
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
Review Comment:
Will this be faster and more elegant in C, or even with a single `for` loop, since you only seem to need `sum(lengths)` ?
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
+ basic_stream <- .Call(nanoarrow_c_basic_array_stream, batches, schema, FALSE)
+ convert_array_stream(basic_stream, to = to, size = sum(lengths))
+}
+
+#' @rdname convert_array_stream
+#' @export
+collect_array_stream <- function(array_stream, n = Inf, schema = NULL,
+ validate = TRUE) {
+ stopifnot(
+ inherits(array_stream, "nanoarrow_array_stream")
+ )
+
+ if (is.null(schema)) {
+ schema <- .Call(nanoarrow_c_array_stream_get_schema, array_stream)
+ }
+
batches <- vector("list", 1024L)
n_batches <- 0L
get_next <- array_stream$get_next
- while (!is.null(array <- get_next(schema, validate = FALSE)) && (n_batches < n)) {
+ while (!is.null(array <- get_next(schema, validate = validate)) && (n_batches < n)) {
Review Comment:
I prefer a `break` inside the loop for the `n_batches < n` case, it also makes it very clear when the check occurs.
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
+ basic_stream <- .Call(nanoarrow_c_basic_array_stream, batches, schema, FALSE)
+ convert_array_stream(basic_stream, to = to, size = sum(lengths))
Review Comment:
Maybe something like this, to avoid the tail recursion and extra work?
```suggestion
batches <- collect_array_stream(
array_stream,
n,
schema = schema,
validate = FALSE
)
# We are guaranteed to have only one batch here:
stopifnot(length(batches) == 1)
.Call(nanoarrow_c_convert_array, batches[[1]], to)
```
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
+ basic_stream <- .Call(nanoarrow_c_basic_array_stream, batches, schema, FALSE)
+ convert_array_stream(basic_stream, to = to, size = sum(lengths))
+}
+
+#' @rdname convert_array_stream
+#' @export
+collect_array_stream <- function(array_stream, n = Inf, schema = NULL,
+ validate = TRUE) {
+ stopifnot(
+ inherits(array_stream, "nanoarrow_array_stream")
+ )
+
+ if (is.null(schema)) {
+ schema <- .Call(nanoarrow_c_array_stream_get_schema, array_stream)
+ }
+
batches <- vector("list", 1024L)
n_batches <- 0L
get_next <- array_stream$get_next
- while (!is.null(array <- get_next(schema, validate = FALSE)) && (n_batches < n)) {
+ while (!is.null(array <- get_next(schema, validate = validate)) && (n_batches < n)) {
n_batches <- n_batches + 1L
- batches[[n_batches]] <- .Call(nanoarrow_c_convert_array, array, to)
+ batches[[n_batches]] <- array
Review Comment:
Without array doubling, the run time still might be superlinear for very many batches. I have no idea if this is of practical relevance, though.
``` r
fill_list <- function(n) {
out <- vector("list", 1024)
for (i in seq_len(n)) {
out[[i]] <- i
}
NULL
}
bench::mark(fill_list(1e6), fill_list(2e6), fill_list(4e6), fill_list(8e6))
#> Warning: Some expressions had a GC in every iteration; so filtering is
#> disabled.
#> # A tibble: 4 × 6
#> expression min median `itr/sec` mem_alloc `gc/sec`
#> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl>
#> 1 fill_list(1e+06) 446.17ms 453.62ms 2.20 165.33MB 2.20
#> 2 fill_list(2e+06) 1.38s 1.38s 0.723 334.95MB 2.17
#> 3 fill_list(4e+06) 4.72s 4.72s 0.212 663.35MB 1.69
#> 4 fill_list(8e+06) 8.42s 8.42s 0.119 1.28GB 1.42
```
<sup>Created on 2023-08-20 with [reprex v2.0.2](https://reprex.tidyverse.org)</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-nanoarrow] paleolimbot commented on a diff in pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279#discussion_r1300210580
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
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-nanoarrow] codecov-commenter commented on pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279#issuecomment-1682898775
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/279?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#279](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/279?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7facd01) into [main](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/ad83497bc8daaec17005fa92b8690c97195bef52?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ad83497) will **decrease** coverage by `0.04%`.
> The diff coverage is `94.11%`.
```diff
@@ Coverage Diff @@
## main #279 +/- ##
==========================================
- Coverage 87.10% 87.06% -0.04%
==========================================
Files 66 63 -3
Lines 10226 9875 -351
==========================================
- Hits 8907 8598 -309
+ Misses 1319 1277 -42
```
| [Files Changed](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [r/R/convert-array-stream.R](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ci9SL2NvbnZlcnQtYXJyYXktc3RyZWFtLlI=) | `97.29% <94.11%> (+0.52%)` | :arrow_up: |
... and [3 files with indirect coverage changes](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/279/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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-nanoarrow] paleolimbot commented on a diff in pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279#discussion_r1300218538
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
+ basic_stream <- .Call(nanoarrow_c_basic_array_stream, batches, schema, FALSE)
+ convert_array_stream(basic_stream, to = to, size = sum(lengths))
+}
+
+#' @rdname convert_array_stream
+#' @export
+collect_array_stream <- function(array_stream, n = Inf, schema = NULL,
+ validate = TRUE) {
+ stopifnot(
+ inherits(array_stream, "nanoarrow_array_stream")
+ )
+
+ if (is.null(schema)) {
+ schema <- .Call(nanoarrow_c_array_stream_get_schema, array_stream)
+ }
+
batches <- vector("list", 1024L)
n_batches <- 0L
get_next <- array_stream$get_next
- while (!is.null(array <- get_next(schema, validate = FALSE)) && (n_batches < n)) {
+ while (!is.null(array <- get_next(schema, validate = validate)) && (n_batches < n)) {
n_batches <- n_batches + 1L
- batches[[n_batches]] <- .Call(nanoarrow_c_convert_array, array, to)
+ batches[[n_batches]] <- array
Review Comment:
It's a good point...I think if this becomes limiting the entire implementation should probably be moved to C or C++, which would also save the overhead of exposing arrays as external pointers. I added a comment to make sure anybody touching this code in the future knows about 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-nanoarrow] paleolimbot commented on a diff in pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279#discussion_r1300212697
##########
r/R/convert-array-stream.R:
##########
@@ -62,26 +68,52 @@ convert_array_stream <- function(array_stream, to = NULL, size = NULL, n = Inf)
)
}
+ # Collect all batches into a list()
+ batches <- collect_array_stream(
+ array_stream,
+ n,
+ schema = schema,
+ validate = FALSE
+ )
+
+ # If there is exactly one batch, use convert_array()
+ if (length(batches) == 1L) {
+ return(.Call(nanoarrow_c_convert_array, batches[[1]], to))
+ }
+
+ # Otherwise, compute the final size, create another array stream,
+ # and call convert_array_stream() with a known size.
+ lengths <- vapply(
+ batches,
+ # Use custom accessor because array$length in a loop is slow
+ function(array) {
+ .Call(nanoarrow_c_array_proxy, array, NULL, FALSE)$length
+ },
+ double(1)
+ )
+ basic_stream <- .Call(nanoarrow_c_basic_array_stream, batches, schema, FALSE)
+ convert_array_stream(basic_stream, to = to, size = sum(lengths))
Review Comment:
I did something slightly different because at this point `length(batches)` is guaranteed *not* to be 1. Hopefully the change makes that more obvious + faster + clearer!
--
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-nanoarrow] paleolimbot commented on pull request #279: refactor(r): Use `basic_array_stream()` to improve array stream to data.frame conversion
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #279:
URL: https://github.com/apache/arrow-nanoarrow/pull/279#issuecomment-1686482584
Thank you for the review!
Documentation and Packaging failures are a sphinx update issue (#284).
--
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