ARROW-9405: [R] Switch to cpp11 #7819

romainfrancois · 2020-07-22T15:25:11Z

No description provided.

github-actions · 2020-07-22T16:06:13Z

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

nealrichardson

Excited to see this come together. The merge conflict with master is probably that I deleted a cpp function and you adapted it to cpp11.

r/tests/testthat/test-Array.R

r/R/array.R

bkietz

some style/lint comments

r/src/memorypool.cpp

r/src/arrow_rcpp.h

bkietz

Some more style/lint comments

r/src/array_from_vector.cpp

r/src/arrow_rcpp.h

r/src/buffer.cpp

r/src/arrow_rcpp.h

r/src/schema.cpp

kszucs · 2020-07-27T09:53:29Z

@romainfrancois please rebase, it has some conflicts with the master.

wesm · 2020-07-28T16:28:34Z

Since this has a bunch of merge commits it will have to be git merge --squash'd to clean things up, let me know if I can help

wesm · 2020-07-28T21:58:28Z

@romainfrancois I squashed, be sure to git fetch origin && git reset --hard origin/cpp11

nealrichardson · 2020-08-04T15:26:05Z

r/R/schema.R

    names = function() {
      out <- Schema__field_names(self)
-      # Hack: Rcpp should set the encoding
      Encoding(out) <- "UTF-8"
      out
    },


If cpp11 is doing what it claims wrt unicode, you can remove this and just

names = function() Schema__field_names(self),

is there a test ?

nealrichardson

A few notes. I think you can also grep for Rf_translateCharUTF8 in r/src and remove those coercions since cpp11 should handle that automatically

r/README.md

r/src/arrow_rcpp.h

r/src/arrow_types.h

nealrichardson · 2020-08-04T15:31:42Z

@github-actions autotune everything

r/src/array_from_vector.cpp

r/src/arrow_cpp11.h

nealrichardson

Not a thorough review, just scanned for a few things to make sure to resolve before we merge

r/tests/testthat/test-buffer.R

r/tests/testthat/test-python.R

nealrichardson · 2020-08-18T15:22:08Z

r/DESCRIPTION

@@ -50,6 +49,8 @@ Suggests:
    rmarkdown,
    testthat,
    tibble
+Remotes:
+    r-lib/cpp11#97


When we're ready to merge this, we should vendor cpp11. I made https://issues.apache.org/jira/browse/ARROW-9786 to go back to the released cpp11 before we do the 2.0.0 release (October-ish).

r/src/array_from_vector.cpp

romainfrancois · 2020-08-19T13:36:09Z

I'm trying to debug what's wrong with the python tests that I previously silenced in this PR:

That looks fine:

library(reticulate)
#> Warning: package 'reticulate' was built under R version 4.0.2
pa <- reticulate::import("pyarrow")
py <- pa$array(c(1, 2, 3))
str(py)
#> [
#>   1,
#>   2,
#>   3
#> ]
class(py)
#> [1] "pyarrow.lib.DoubleArray"        "pyarrow.lib.FloatingPointArray"
#> [3] "pyarrow.lib.NumericArray"       "pyarrow.lib.Array"             
#> [5] "pyarrow.lib._PandasConvertible" "python.builtin.object"

But then if I also load arrow I'm getting a segfault on pa$array() call:

library(reticulate)
library(arrow)
pa <- reticulate::import("pyarrow")
py <- pa$array(c(1, 2, 3))

romainfrancois · 2020-08-19T13:39:17Z

The error seems to happen on this call:

x$`_export_to_c`(array_ptr, schema_ptr)

although there seems to be something there:

Browse[2]> x$`_export_to_c`
<built-in method _export_to_c of pyarrow.lib.DoubleArray>

romainfrancois · 2020-08-19T14:11:00Z

And I can confirm that this works on master locally:

library(reticulate)
#> Warning: package 'reticulate' was built under R version 4.0.2
library(arrow)
#> 
#> Attaching package: 'arrow'
#> The following object is masked from 'package:utils':
#> 
#>     timestamp
pa <- reticulate::import("pyarrow")
py <- pa$array(c(1, 2, 3))
py
#> Array
#> <double>
#> [
#>   1,
#>   2,
#>   3
#> ]

^{Created on 2020-08-19 by the reprex package (v0.3.0.9001)}

More investigation needed.

nealrichardson · 2020-08-19T14:37:41Z

r/src/arrow_cpp11.h

+namespace unsafe {
+
+inline SEXP utf8_r_string(SEXP s) {
+  if (!IS_UTF8(s) && !IS_ASCII(s)) {


Are you sure these utilities are necessary? translateCharUTF8 also checks IS_UTF8 and IS_ASCII and exits early: https://github.com/wch/r-source/blob/122dcf452ec5eacdd66e165457985bded1af4fee/src/main/sysutils.c#L1085

They might not all be necessary, I'm trying to avoid the Rf_mkChar(Rf_translateCharUTF8()) combo, which would unnecessarily make a SEXP

This is thin for now, but this might evolve into perhaps something like cpp11::utf8_strings class that would eagerly convert at construction time.

…pp11::warning() that fails locally for some reason

…tring to std::string)

- TraverseDotsNoName

…to double.

nealrichardson

I just pulled this and ran locally, and compile time (single-threaded) for the r/src bindings is cut in half: master takes 93 seconds on my machine, and this takes 48 seconds.

Thanks for doing this!

Closes apache#7819 from romainfrancois/cpp11 Lead-authored-by: Romain Francois <romain@rstudio.com> Co-authored-by: Romain François <romain@rstudio.com> Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com> Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>

nealrichardson changed the title ~~[R]: Switch from Rcpp to cpp11~~ ARROW-9405: [R] Switch to cpp11 Jul 22, 2020

bkietz requested review from nealrichardson and bkietz and removed request for nealrichardson July 23, 2020 13:07

nealrichardson reviewed Jul 23, 2020

View reviewed changes

r/tests/testthat/test-Array.R Show resolved Hide resolved

r/R/array.R Outdated Show resolved Hide resolved

bkietz requested changes Jul 23, 2020

View reviewed changes

kszucs force-pushed the master branch from a06a0f4 to baf2094 Compare July 24, 2020 12:28

bkietz requested changes Jul 24, 2020

View reviewed changes

wesm force-pushed the cpp11 branch from 306cb2b to 9c63e7a Compare July 28, 2020 21:57

romainfrancois force-pushed the cpp11 branch 2 times, most recently from ba72863 to d4117ae Compare August 4, 2020 13:53

nealrichardson reviewed Aug 4, 2020

View reviewed changes

r/README.md Outdated Show resolved Hide resolved

r/src/arrow_rcpp.h Outdated Show resolved Hide resolved

r/src/arrow_types.h Outdated Show resolved Hide resolved

romainfrancois force-pushed the cpp11 branch from 4a01103 to 80653e1 Compare August 6, 2020 12:09

bkietz reviewed Aug 7, 2020

View reviewed changes

romainfrancois force-pushed the cpp11 branch from 1a2befd to 40c82a2 Compare August 17, 2020 08:37

romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Aug 18, 2020

added comment for apache#7819 (comment)

fcbf1c1

romainfrancois added a commit to romainfrancois/arrow that referenced this pull request Aug 18, 2020

added comment for apache#7819 (comment)

a34b216

romainfrancois force-pushed the cpp11 branch from e38573c to 58569cc Compare August 18, 2020 12:12

nealrichardson requested changes Aug 18, 2020

View reviewed changes

nealrichardson reviewed Aug 19, 2020

View reviewed changes

romainfrancois added 22 commits August 24, 2020 15:59

Converter_Dictionary cannot ingest in parallel, and avoid a form of c…

cca34a3

…pp11::warning() that fails locally for some reason

added comment for apache#7819 (comment)

f244547

using master cpp11

021c2b9

-W-sign-compare finding

a7d8a91

using r-lib/cpp11#97

dab5115

g++ needs space

d8eb365

one more use of arrow::r::TraverseDots()

3f45697

traverse dots automatically using utf8 (thanks to conversion from r_s…

4d4d220

…tring to std::string)

TraverseDots(cpp11::list dots, ...)

61b0aa5

- TraverseDotsNoName

.size() rather than XLENGTH()

2b6ea2b

avoid conversion to utf8 when not needed

6cdb4f3

Doing conversions upftont, so using UnsafeAppend

c5bf268

Buffer only parameterized by vector type

d6146c0

re-add ability to create a Buffer from an R complex vector

06c52f0

marking ctor explicit

a78f3c3

explicitly make a complexs vector

f7d1baa

work around cpp11 converting uintptr_t to int where Rcpp did convert …

7eabb63

…to double.

no longer skip python tests

e2669cc

r-lib/cpp11#97 was merged

9ed0846

vendor cpp11

1ea6b99

ignore vendored cpp11 files

701e35e

LinkingTo: self

9a993cc

nealrichardson force-pushed the cpp11 branch from fc6597e to 9a993cc Compare August 24, 2020 23:18

📰 and python test tweak

bb0371b

nealrichardson approved these changes Aug 24, 2020

View reviewed changes

nealrichardson closed this in 55defbf Aug 24, 2020

This was referenced Apr 13, 2022

[R] Switch to cpp11 #25484

Closed

[R] Improve efficiency of decimal casting #20043

Closed

ARROW-9405: [R] Switch to cpp11 #7819

ARROW-9405: [R] Switch to cpp11 #7819

Uh oh!

Conversation

romainfrancois commented Jul 22, 2020 • edited by nealrichardson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 22, 2020

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kszucs commented Jul 27, 2020

Uh oh!

wesm commented Jul 28, 2020

Uh oh!

wesm commented Jul 28, 2020

Uh oh!

nealrichardson Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

romainfrancois Aug 4, 2020

Choose a reason for hiding this comment

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nealrichardson commented Aug 4, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nealrichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nealrichardson Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

romainfrancois commented Aug 19, 2020

Uh oh!

romainfrancois commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romainfrancois commented Aug 19, 2020

romainfrancois commented Jul 22, 2020 •

edited by nealrichardson

Loading

romainfrancois commented Aug 19, 2020 •

edited

Loading