ARROW-13164: [R] altrep vectors from Array with nulls

[romainfrancois]

No description provided.

[github-actions]

https://issues.apache.org/jira/browse/ARROW-13164

[romainfrancois]

follow up after #10593

[romainfrancois]

Thanks for the reviews. I'll apply the relevant changes.

We now have AltrepArrayNoNulls<INTSXP|REALSXP> and AltrepArrayWithNulls<INTSXP|REALSXP> which are not all that different. I believe they can be merged into one template, which would perhaps slightly more involved Dataptr() ... methods...

[romainfrancois]

I finally settled for a single altrep (template) class for both cases (the array has no nulls or the array has nulls).

The altrep object uses:

• data1: an external pointer to the Array (well to a shared pointer to the Array)
• data2: NULL until materialization is needed, then data2 is a standard (and immutable) R vector with the same data

Materialization is needed when:

• we need the DATAPTR and the Array has nulls
• we need a duplicate

Get_region() does not need to materialize the full vector.
Creating the altrep object does not alter the data to impose the R sentinel.

[pitrou]

I'm going to say that the AltrepArrayPrimitive seems very difficult to read and understand to me.

I would recommend:

1. write extensive comments and explanations (including hyperlinks to R-related docs where necessary)
2. choose naming wisely
3. make sure no unused functionality or code paths are left
4. make internal helper methods private, to better stress what is the "public" API
5. also, make sure we're not doing any logic errors (if the user requests a writable array, do we really want to return a pointer to the immutable Arrow data?)

[romainfrancois]

Changed Dataptr(writeable = TRUE) so that the data is materialized.

[romainfrancois]

🤔 I think now that means that if the data has been materialized, a Dataptr(writeable = TRUE) might have happened, and maybe the data has been written into. To be on the safe side, perhaps we should now treat the Array as obsolete and only ever use data2

[nealrichardson]

maybe the data has been written into

We can write tests for this behavior, yeah?

[romainfrancois]

The AltrepArrayPrimitive class now marks the altrep object as immutable. This makes this "a bad idea" to call Dataptr(writeable = TRUE) on it, and it should rather be accessed read-only.

However, if we make this an error, this fails in many many places, including identical() so we can't do that.

[nealrichardson]

@github-actions crossbow submit -g r

[github-actions]

Revision: 763781c

Submitted crossbow builds: ursacomputing/crossbow @ actions-806

Task	Status
conda-linux-gcc-py36-cpu-r40
conda-linux-gcc-py37-cpu-r41
conda-osx-clang-py36-r40
conda-osx-clang-py37-r41
conda-win-vs2017-py36-r40
conda-win-vs2017-py37-r41
homebrew-r-autobrew
test-r-depsource-auto
test-r-depsource-system
test-r-devdocs
test-r-gcc-11
test-r-install-local
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-base-latest
test-r-rstudio-r-base-3.6-bionic
test-r-rstudio-r-base-3.6-centos7-devtoolset-8
test-r-rstudio-r-base-3.6-centos8
test-r-rstudio-r-base-3.6-opensuse15
test-r-rstudio-r-base-3.6-opensuse42
test-r-ubuntu-21.04
test-r-version-compatibility
test-r-versions
test-ubuntu-18.04-r-sanitizer

[nealrichardson]

I think that's all clean. @jonkeane do you want to do any benchmarking before we merge this?

[nealrichardson]

@ursabot please benchmark lang=R

[ursabot]

Benchmark runs are scheduled for baseline = f60bc12 and contender = 763781c. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Provided benchmark filters do not have any benchmark groups to be executed on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Finished ⬇️2.19% ⬆️1.46%] ursa-i9-9960x
[Skipped ⚠️ Only ['C++', 'Java'] langs are supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

[jonkeane]

Yeah, we should run these manually. I'll try to get to that tomorrow. None of the array conversion benchmarks have been wired up in the benchmarks package, so we don't have them available on conbench right now.

[jonkeane]

OK, I just ran Array to R benchmarks locally, and the results are very impressive. I've added in other types (even though they are not (yet) backed by altrep). I've also tested both chunked and non-chunked arrays, and with nulls and without.

TL;DR:

For floats + ints we see huge speed ups (~10x faster) in both arrays with and without nulls as well as in arrays and chunked arrays (at least in the case where there is a single chunk*).

I'm still investigating what's going on with the two fannie parquet file reads that show up as regressions. If anything, this should have sped them up.

• – we don't yet have fixtures with chunked arrays with multiple chunks. But I simulated this with a csv (which does come in with multiple chunks) and multi-chunk arrays aren't (yet) supported:

library(arrow)
nyctaxi<- read_csv_arrow("~/repos/ab_store/data/nyctaxi_2010-01.csv.gz", as_data_frame=FALSE)
chunked_array<- as.vector(nyctaxi[[5]])
.Internal(inspect(chunked_array))
#> @7fb218000000 14 REALSXP g0c7 [REF(4)] (len=14863778, tl=0) 0.75,5.9,4,4.7,0.6,...array<- as.vector(nyctaxi[[5]]$chunk(1))
.Internal(inspect(array))
#> @7fb20dc3a6e0 14 REALSXP g0c0 [REF(65535)] arrow::Array<double, NONULL> len=5738, Array=<0x7fb25d5080d8>#> @7fb20dc3a670 22 EXTPTRSXP g0c0 [REF(4)]

[jonkeane]

And here's the report:

altvec-with-nulls.html.zip

[nealrichardson]

• multi-chunk arrays aren't (yet) supported

That's ARROW-13111. ARROW-13112 is for supporting other types. If I understand correctly, we're conceptually close on those since we're implementing the get_region and elt methods for integer and float here, just ("just"!) need to [something about templates] and do the same for other types and for ChunkedArrays using our existing conversion code.