GH-45949: [R] Fix CRAN warnings for 19.0.1 about compiled code #45951

jonkeane · 2025-03-27T03:09:05Z

This gets rid of OBJECT, DATAPTR has been replaced with INTEGER(), REAL(), etc. though strings are more complicated. I will fully admit that this C++ is stretching my comfort zone, so might include obviously wrong things!

CI is currently failing, but I'm not totally sure yet if that means the code changes here are wrong or if maybe these allow us to have slightly different assumptions about materialization (see #45951 (comment))

I've also requested reviews broadly for folks I know have been around this code before, I appreciate any effort that y'all can spare 🙏

GitHub Issue: [R] Fix CRAN warnings for 19.0.1 about compiled code #45949

jonkeane · 2025-03-29T02:28:22Z

r/src/altrep.cpp

+    R_xlen_t len = Rf_xlength(x);
+
+    for (R_xlen_t i = 0; i < len; i++) {
+      SEXP str_elt = reinterpret_cast<SEXP>(STRING_ELT(x, i));
+      out[i] = str_elt;


Something about this isn't quite right, because the test here (and other assertions that strings are materialized) fail:

arrow/r/tests/testthat/test-altrep.R

Line 249 in 3c63f37

expect_true(test_arrow_altrep_is_materialized(altrep))

But I haven't yet figured out if this is a real problem with this code change, or maybe it's an assumption in the tests that no longer holds?

@nealrichardson @paleolimbot y'all might have thoughts ?

As a follow-on for myself that line 1319 is duplicative and should be removed (but I don't want to outdate ^^^ just yet)

DATAPTR and STRING_ELT work a bit different for altrep so it seems reasonable to me to assume we were implicitly materializing before and no longer are with STRING_ELT. It seems like this is an assumption of the test that no longer holds and the expectation could be removed.

jonkeane · 2025-03-29T13:33:45Z

r/src/arrow_types.h

+    } else if (TYPEOF(vec) == STRSXP) {
+      cpp11::writable::strings out(Rf_xlength(vec));
+      R_xlen_t len = Rf_xlength(vec);
+
+      for (R_xlen_t i = 0; i < len; i++) {
+        SEXP str_elt = reinterpret_cast<SEXP>(STRING_ELT(vec, i));
+        out[i] = str_elt;
+      }
+
+      return out;


I believe this chunk is necessary to return the array of strings here. But oddly(??) when I tried this block as simply return STRING_ELT(vec, 0); (which would return just the first element IIUC), all tests passed. So maybe I misunderstand what's happening in the MutableBuffer there and we actually only need an object of the right type? Or we don't have test coverage that ensures that the full vector is there?

Just doing return STRING_ELT(vec, 0) actually makes sense to me since it looks like we just need a pointer to the same address as what we'd get with DATAPTR. Seems like that's what STRING_ELT(vec, 0) should accomplish. It does seem like a strange way to do it but it also seems like what we're doing here is already breaking the rules CRAN wants us to play by. If it works, I'm +1.

Ok, I've also commented this to be explicit for the next person

nealrichardson · 2025-03-30T20:22:10Z

I don't understand this code well enough to have much to say. I did look at the part of WRE that the CRAN check points to, and it suggests using DATAPTR_RO instead of DATAPTR: https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Moving-into-C-API-compliance

Is that not an option for us?

jonkeane · 2025-03-30T20:33:19Z

suggests using DATAPTR_RO instead of DATAPTR: https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Moving-into-C-API-compliance

Is that not an option for us?

It is an option in one place and I did use it there, but not all: I would get segfaults/illegal access in the places that I didn't use it. If I'm reading the code correctly, those are places where we are actually mutating in place

amoeba

Thanks for working on this. I took a look through and left a couple of comments. Once the altrep test failures get figured out I'll be a +1 on this.

jonkeane · 2025-03-31T12:28:43Z

r/tests/testthat/test-altrep.R

-  # because there are no nulls, DATAPTR() does not materialize
+  # because there are nulls, DATAPTR() does materialize


This comment change and the one below are not behavior changes with this PR, but I think the comments were simply wrong (either old or copy pasted). I've tried to correct them to be accurate descriptions of what's going on (but see "does not materialize" and then two lines later expect_true(test_arrow_altrep_is_materialized(altrep)) is at odds with each other

You're correct here that my earlier comment was simply wrong 🙂

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

jonkeane · 2025-04-05T14:49:49Z

@github-actions crossbow submit -g r

github-actions · 2025-04-05T14:52:27Z

Revision: a4b457d

Submitted crossbow builds: ursacomputing/crossbow @ actions-97a8d02cea

Task	Status
r-binary-packages
r-recheck-most
test-r-arrow-backwards-compatibility
test-r-clang-sanitizer
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-extra-packages
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-sanitizer
test-r-linux-valgrind
test-r-macos-as-cran
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-release-latest
test-r-rocker-r-ver-latest
test-r-rstudio-r-base-4.1-opensuse155
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions

jonkeane · 2025-04-05T15:41:31Z

Failres: test-r-rstudio-r-base-4.1-opensuse155 and test-r-offline-maximal are unrelated / being fixed elsewhere.

amoeba

+1

paleolimbot

Thanks!

paleolimbot · 2025-04-05T19:08:47Z

r/src/altrep.cpp

@@ -531,7 +552,7 @@ struct AltrepFactor : public AltrepVectorBase<AltrepFactor> {
      SEXP copy = PROTECT(Rf_allocVector(INTSXP, size));

      // copy the data from the array, through Get_region
-      Get_region(alt, 0, size, reinterpret_cast<int*>(DATAPTR(copy)));
+      Get_region(alt, 0, size, reinterpret_cast<int*>(INTEGER(copy)));


Suggested change

Get_region(alt, 0, size, reinterpret_cast<int*>(INTEGER(copy)));

Get_region(alt, 0, size, INTEGER(copy));

Ah yes, thanks for that reminder, I found a few others too

paleolimbot · 2025-04-05T19:19:34Z

r/src/altrep.cpp

+      SEXP str_elt = reinterpret_cast<SEXP>(STRING_ELT(x, i));
+      out[i] = str_elt;


Thanks for checking that! I don't recall any tests that checked the whether something was materialized more than once but it was quite a long time ago 🙂

paleolimbot · 2025-04-05T19:20:23Z

r/tests/testthat/test-altrep.R

-  # DATAPTR() should always materialize for strings
+  # DATAPTR() does not materialize


jonkeane · 2025-04-05T22:03:24Z

@assignUser would it be possible to pull this into 20? We will need to patch it in our CRAN release regardless, but it would be nice to have it on the actual release.

assignUser · 2025-04-05T22:13:00Z

Of course!

conbench-apache-arrow · 2025-04-06T04:52:47Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 34a984c.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

This gets rid of `OBJECT`, `DATAPTR` has been replaced with `INTEGER()`, `REAL()`, etc. though strings are more complicated. I will fully admit that this C++ is stretching my comfort zone, so might include obviously wrong things! CI is currently failing, but I'm not totally sure yet if that means the code changes here are wrong or if maybe these allow us to have slightly different assumptions about materialization (see #45951 (comment)) I've also requested reviews broadly for folks I know have been around this code before, I appreciate any effort that y'all can spare 🙏 * GitHub Issue: #45949 Lead-authored-by: Jonathan Keane <jkeane@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

…pache#45951) This gets rid of `OBJECT`, `DATAPTR` has been replaced with `INTEGER()`, `REAL()`, etc. though strings are more complicated. I will fully admit that this C++ is stretching my comfort zone, so might include obviously wrong things! CI is currently failing, but I'm not totally sure yet if that means the code changes here are wrong or if maybe these allow us to have slightly different assumptions about materialization (see apache#45951 (comment)) I've also requested reviews broadly for folks I know have been around this code before, I appreciate any effort that y'all can spare 🙏 * GitHub Issue: apache#45949 Lead-authored-by: Jonathan Keane <jkeane@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Jonathan Keane <jkeane@gmail.com>

jonkeane requested a review from thisisnic as a code owner March 27, 2025 03:09

github-actions bot added Component: R awaiting committer review Awaiting committer review labels Mar 27, 2025

jonkeane mentioned this pull request Mar 28, 2025

[R] CRAN packaging checklist for version 19.0.1.1 #45950

Closed

36 tasks

jonkeane commented Mar 29, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting committer review Awaiting committer review awaiting changes Awaiting changes awaiting change review Awaiting change review labels Mar 29, 2025

jonkeane commented Mar 29, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 29, 2025

jonkeane requested review from nealrichardson, paleolimbot, kou, amoeba and romainfrancois March 29, 2025 13:36

amoeba reviewed Mar 31, 2025

View reviewed changes

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 31, 2025

jonkeane commented Mar 31, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Mar 31, 2025

jonkeane requested a review from amoeba March 31, 2025 13:42

jonkeane and others added 4 commits April 5, 2025 09:48

Update r/src/altrep.cpp

7351ce9

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

PR comments

7a85dcf

PR comments

ba72662

More specific static assertions

a4b457d

jonkeane force-pushed the 45949_nonapi_again branch from 310d9c2 to a4b457d Compare April 5, 2025 14:49

jonkeane requested a review from paleolimbot April 5, 2025 14:49

amoeba approved these changes Apr 5, 2025

View reviewed changes

paleolimbot approved these changes Apr 5, 2025

View reviewed changes

github-actions bot added awaiting merge Awaiting merge awaiting review Awaiting review awaiting changes Awaiting changes and removed awaiting change review Awaiting change review awaiting review Awaiting review awaiting merge Awaiting merge labels Apr 5, 2025

Remove a few more extraneous reinterpret_casts

a15918a

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 5, 2025

jonkeane merged commit 34a984c into apache:main Apr 5, 2025
10 checks passed

jonkeane removed the awaiting change review Awaiting change review label Apr 5, 2025

jonkeane mentioned this pull request Apr 5, 2025

[R] Fix CRAN warnings for 19.0.1 about compiled code #45949

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-45949: [R] Fix CRAN warnings for 19.0.1 about compiled code #45951

GH-45949: [R] Fix CRAN warnings for 19.0.1 about compiled code #45951

jonkeane commented Mar 27, 2025 •

edited

Loading

jonkeane Mar 29, 2025

jonkeane Mar 29, 2025

amoeba Mar 31, 2025

jonkeane Mar 29, 2025

amoeba Mar 31, 2025

jonkeane Mar 31, 2025

nealrichardson commented Mar 30, 2025

jonkeane commented Mar 30, 2025

amoeba left a comment

jonkeane Mar 31, 2025

paleolimbot Mar 31, 2025

jonkeane commented Apr 5, 2025

github-actions bot commented Apr 5, 2025

jonkeane commented Apr 5, 2025

amoeba left a comment

paleolimbot left a comment

paleolimbot Apr 5, 2025

jonkeane Apr 5, 2025

paleolimbot Apr 5, 2025

paleolimbot Apr 5, 2025

jonkeane commented Apr 5, 2025

assignUser commented Apr 5, 2025

conbench-apache-arrow bot commented Apr 6, 2025

		# because there are no nulls, DATAPTR() does not materialize
		# because there are nulls, DATAPTR() does materialize

	Get_region(alt, 0, size, reinterpret_cast<int*>(INTEGER(copy)));
	Get_region(alt, 0, size, INTEGER(copy));

		SEXP str_elt = reinterpret_cast<SEXP>(STRING_ELT(x, i));
		out[i] = str_elt;

		# DATAPTR() should always materialize for strings
		# DATAPTR() does not materialize

GH-45949: [R] Fix CRAN warnings for 19.0.1 about compiled code #45951

GH-45949: [R] Fix CRAN warnings for 19.0.1 about compiled code #45951

Conversation

jonkeane commented Mar 27, 2025 • edited Loading

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

nealrichardson commented Mar 30, 2025

jonkeane commented Mar 30, 2025

amoeba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonkeane commented Apr 5, 2025

github-actions bot commented Apr 5, 2025

jonkeane commented Apr 5, 2025

amoeba left a comment

Choose a reason for hiding this comment

paleolimbot left a 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

jonkeane commented Apr 5, 2025

assignUser commented Apr 5, 2025

conbench-apache-arrow bot commented Apr 6, 2025

jonkeane commented Mar 27, 2025 •

edited

Loading