feat(r): Add infrastructure methods for dealing with chunked values #75

paleolimbot · 2023-11-16T01:44:10Z

Not polished yet! I had intended to start on the actual vctr bit a few weeks ago but didn't get far past some very boring details.

anthonynorth · 2023-11-17T01:24:33Z

r/geoarrow/R/vctr.R

+new_geoarrow_vctr <- function(chunks, schema) {
+  offsets <- .Call(geoarrow_c_vctr_chunk_offsets, chunks)
+  structure(
+    seq_len(offsets[length(offsets)]),
+    schema = schema,
+    chunks = chunks,
+    chunk_offsets = offsets,
+    class = c("geoarrow_vctr", "wk_vctr")
+  )


Could we store just the array stream and wk attributes (crs, geodesic)? I think we can retrieve schema and offsets from the array stream when they're required?

I'm pretty sure we need list() of nanoarrow_array here and offsets so that we can efficiently subset (see the bit I added). Once you have a stream, you don't know how many chunks or elements there are and you loose random access. I definitely may have missed something though!

For crs and geodesic I'd envisioned keeping those with the schema (i.e., modifying them via wk_set_crs() or wk_set_geodesic() would modify the schema instead of set an attribute on the vctr). Also not sure I thought that all the way through or not!

For crs and geodesic I'd envisioned keeping those with the schema (i.e., modifying them via wk_set_crs() or wk_set_geodesic() would modify the schema instead of set an attribute on the vctr). Also not sure I thought that all the way through or not!

That makes sense. I'd forgotten these are available in the schema.

I'm pretty sure we need list() of nanoarrow_array here and offsets so that we can efficiently subset (see the bit I added). Once you have a stream, you don't know how many chunks or elements there are and you loose random access. I definitely may have missed something though!

Ok.

anthonynorth · 2023-11-17T01:31:35Z

r/geoarrow/R/vctr.R

+    stop("Can't resolve non-slice geoarrow_vctr to nanoarrow_array_stream")
+  }
+
+  chunk1


Ok, it was a bit of a rabbit hole to finish my thought here, but this was basically the place where the slicing and subsetting of the chunks had to happen.

paleolimbot · 2023-11-18T03:04:57Z

Ok, I finished the thought that I started. Definitely let me know if this doesn't make sense...it's only ever been in my head!

r/geoarrow/R/vctr.R

anthonynorth · 2023-11-22T09:51:45Z

r/geoarrow/R/vctr.R

+
+  # Calculate first and last slice information
+  first_index <- slice[1] - 1L
+  end_index <- first_index + slice[2]


Is end_index always max(offsets)?

It would be max(offsets) if the slice were 1:length(n), but most of what I'm trying to support here is something like head(x, 10) (i.e., what happens when you print).

anthonynorth · 2023-11-22T10:37:39Z

r/geoarrow/R/vctr.R

+    )
+  }
+
+  # Calculate first and last slice information


Can I suggest 1-indexing for these variables? We need 1-indexed values for slicing offsets and chunks. We need 0-indexed offset nanoarrow::nanoarrow_array_modify() calls.

I think 1-indexing simplifies this slightly.

I think they both have their advantages...you are right that mixing them introduces a lot of confusion. I think the main motivation was to avoid too many +1 and -1s in C code, where these things are being done in a tight loop. At the R level the +1s and -1s are not (or shouldn't be!) on anything other than a scalar.

anthonynorth · 2023-11-22T10:50:08Z

r/geoarrow/R/vctr.R

+
+# Utilities for vctr methods
+
+vctr_resolve_chunk <- function(x, offsets) {


Do we need to support a vector x, or is a scalar sufficient?

Reason I ask: I think we can resolve chunks with a trivial R function for this for scalar x

resolve_chunk <- function(x, offsets) { if (x < offsets[1L] || x >= offsets[length(offsets)]) return(NA_integer_) sum(x >= offsets[-length(offsets)]) - 1L }

A scalar is probably fine, although the binary search is I think important because of the log() complexity. The vectorizing was useful for testing but is probably not important for performance.

anthonynorth · 2023-11-22T10:55:30Z

r/geoarrow/src/r-vctr.c

+}
+
+SEXP geoarrow_c_vctr_as_slice(SEXP indices_sexp) {
+  if (TYPEOF(indices_sexp) == INTSXP) {


Inverting this condition & early return reduces the nesting here

Suggested change

if (TYPEOF(indices_sexp) == INTSXP) {

if (TYPEOF(indices_sexp) != INTSXP) return R_NilValue;

anthonynorth · 2023-11-22T11:07:08Z

r/geoarrow/src/r-vctr.c

+  return chunk_indices_sexp;
+}
+
+SEXP geoarrow_c_vctr_as_slice(SEXP indices_sexp) {


Is a slice the starting 1-index, and length (ignoring the 0-length special case)?

Could we simplify this logic in 2 parts?

check the indices vector is an integer sequence, or 0-length

return the 2-vector c(start_index, length)

Assuming I've not missed anything, both of these have simple and fast R implementations if the C-level implementation isn't needed for anything else.

is_sequence <- all(indices == seq.int(indices[1], indices[1] + length(indices) - 1L)) slice <- if (length(indices) == 0) c(NA_integer_, 0L) else c(indices[1], length(indices))

This is again me being a little paranoid about huge input. The main thing that can happen here is that indices is 1:1e9 or something: R represents that as ALTREP. The C versions here avoids altrep expansion:

indices <- 1:1e6 .Internal(inspect(indices)) #> @12aa00770 13 INTSXP g0c0 [REF(65535)] 1 : 1000000 (compact) is_sequence <- all(indices == seq.int(indices[1], indices[1] + length(indices) - 1L)) .Internal(inspect(indices)) #> @12aa00770 13 INTSXP g0c0 [REF(65535)] 1 : 1000000 (expanded) indices <- 1:1e6 .Internal(inspect(indices)) #> @12af9b5d0 13 INTSXP g0c0 [REF(65535)] 1 : 1000000 (compact) geoarrow:::vctr_as_slice(indices) #> Error in eval(expr, envir, enclos): object 'vctr_as_slice' not found .Internal(inspect(indices)) #> @12af9b5d0 13 INTSXP g1c0 [MARK,REF(65535)] 1 : 1000000 (compact)

indices <- 1:1e9 bench::mark( r = { is_sequence <- all(indices == seq.int(indices[1], indices[1] + length(indices) - 1L)) slice <- if (length(indices) == 0) c(NA_integer_, 0L) else c(indices[1], length(indices)) }, c = geoarrow:::vctr_as_slice(indices) ) #> Warning: Some expressions had a GC in every iteration; so filtering is #> disabled. #> # A tibble: 2 × 6 #> expression min median `itr/sec` mem_alloc `gc/sec` #> <bch:expr> <bch:tm> <bch:tm> <dbl> <bch:byt> <dbl> #> 1 r 5.75s 5.75s 0.174 11.18GB 0.521 #> 2 c 1.28s 1.28s 0.778 1.59MB 0

anthonynorth · 2023-11-22T11:23:27Z

r/geoarrow/src/r-vctr.c

+
+#include "geoarrow.h"
+
+SEXP geoarrow_c_vctr_chunk_offsets(SEXP array_list) {


Do we expect array_list to be large? If not, should this logic be moved to R? The overhead should be tiny if array_list is small.

chunk_lengths <- vapply(chunks, function(x) x$length, integer(1L)) c(0L, cumsum(chunk_lengths))

You're right that the list is usually small. There can be situations where the chunk size is 1024 and there are 100 million results, though. In the Arrow R package we do some occasional looping over columns and chunks and it's caused some performance problems that have been difficult to fix. I know it's a bit overkill for most situations, but I do want to minimize looping over chunks.

I'm definitely not thinking of 100 million length geometry vectors!

anthonynorth · 2023-11-22T11:43:24Z

Ok, I finished the thought that I started. Definitely let me know if this doesn't make sense...it's only ever been in my head!

I finally got around to reading. Code makes sense. The indexing logic in as_nanoarrow_array_stream.geoarrow_vctr() took me a moment or 5 to understand though.

I've only found 1 possible bug (or my misunderstanding), in as_nanoarrow_array_stream.geoarrow_vctr(). I've left a suggestion for that.

I haven't read the unit tests.

paleolimbot · 2023-11-26T03:04:50Z

Thank you for taking a look, and sorry for taking so long to circle back! I got this to a place where my next PR is unblocked (Arrow R package integration), although I do think a few of the internal could be simplified/documented/refactored to either address your comments above or ensure that the next reader has to spend less time to figure out what's going on!

paleolimbot mentioned this pull request Nov 16, 2023

[R]: geoarrow_vctr class #70

Open

anthonynorth reviewed Nov 17, 2023

View reviewed changes

paleolimbot marked this pull request as ready for review November 18, 2023 02:58

anthonynorth reviewed Nov 22, 2023

View reviewed changes

r/geoarrow/R/vctr.R Outdated Show resolved Hide resolved

anthonynorth reviewed Nov 22, 2023

View reviewed changes

paleolimbot added 10 commits November 25, 2023 21:57

dump of vctr things

ad2cc3a

basic tests

2257e88

tests

d4656ba

callentries generator

da91d46

fix nesting

3dc062c

use slice helper

f9a1ed4

add test for as_geoarrow_vctr()

db3f980

wire up handle

9ced1ce

methods

fd89d4e

fix

80cb3bd

paleolimbot force-pushed the r-vctr-rough branch from 94f05c9 to 80cb3bd Compare November 26, 2023 02:42

paleolimbot mentioned this pull request Nov 26, 2023

[R] processing nanoarrow streams with geoarrow column #78

Closed

paleolimbot merged commit 7ae1089 into geoarrow:main Nov 26, 2023

paleolimbot deleted the r-vctr-rough branch November 26, 2023 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(r): Add infrastructure methods for dealing with chunked values #75

feat(r): Add infrastructure methods for dealing with chunked values #75

paleolimbot commented Nov 16, 2023

anthonynorth Nov 17, 2023

paleolimbot Nov 18, 2023

anthonynorth Nov 22, 2023

anthonynorth Nov 17, 2023

paleolimbot Nov 18, 2023

paleolimbot commented Nov 18, 2023

anthonynorth Nov 22, 2023

paleolimbot Nov 26, 2023

anthonynorth Nov 22, 2023

paleolimbot Nov 26, 2023

anthonynorth Nov 22, 2023

paleolimbot Nov 26, 2023

anthonynorth Nov 22, 2023

anthonynorth Nov 22, 2023

paleolimbot Nov 26, 2023

anthonynorth Nov 22, 2023

paleolimbot Nov 26, 2023

anthonynorth Nov 26, 2023

anthonynorth commented Nov 22, 2023 •

edited

Loading

paleolimbot commented Nov 26, 2023


		# Utilities for vctr methods

		vctr_resolve_chunk <- function(x, offsets) {

	if (TYPEOF(indices_sexp) == INTSXP) {
	if (TYPEOF(indices_sexp) != INTSXP) return R_NilValue;


		#include "geoarrow.h"

		SEXP geoarrow_c_vctr_chunk_offsets(SEXP array_list) {

feat(r): Add infrastructure methods for dealing with chunked values #75

feat(r): Add infrastructure methods for dealing with chunked values #75

Conversation

paleolimbot commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paleolimbot commented Nov 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonynorth commented Nov 22, 2023 • edited Loading

paleolimbot commented Nov 26, 2023

anthonynorth commented Nov 22, 2023 •

edited

Loading