GH-48631: [R] Non-API calls: 'ATTRIB', 'SET_ATTRIB' #48634

thisisnic · 2025-12-23T08:03:40Z

Rationale for this change

CRAN complains about non-API calls

What changes are included in this PR?

Update those items to use alternative approaches

Are these changes tested?

Well, if the tests pass I think we're good?

Are there any user-facing changes?

Nope

Summary of AI changes

🤖 Generated with Claude Code

Have added comments inline regarding assumptions behind changes, and where I questioned those to try to verify it.

Sources referenced in approach:

Replace ATTRIB, SET_ATTRIB Rdatatable/data.table#7487 - similar fixes
https://cran.r-project.org/doc/manuals/r-release/R-ints.html on DUPLICATE_ATTRIB

GitHub Issue: [R] Non-API calls: 'ATTRIB', 'SET_ATTRIB' #48631

thisisnic · 2025-12-23T08:08:33Z

r/src/altrep.cpp

+    // copy attributes from the altrep object
+    DUPLICATE_ATTRIB(dup, alt);

-    UNPROTECT(2);
+    UNPROTECT(1);


🤖 Note: DUPLICATE_ATTRIB also copies the OBJECT bit and S4 status, which the old code didn't do.

Was unsure if this matters?

I don't know altrep well enough to know the answer, but copying the whole object sounds like it might be detrimental to performance — any idea if that's true?

🤖 Performance impact: Negligible. The OBJECT bit and S4 flag are just bit operations - essentially free compared to the actual attribute list copying that both approaches do.

I queried these a little more and it looks like these are just metadata - the OBJECT bit determines the result of calling is.object() and the S4 bit indicates if the object is an S4 object.

I took a look at https://cran.r-project.org/doc/manuals/r-release/R-ints.html#Attributes-1 and also queried what this bit of the codebase does (answer: wrapper around ChunkedArray to store data when we call e.g. collect() as an ALTREP vector until R actually needs it), and whether we definitely need S4 and OBJECT: apparently we need OBJECT but not S4:

🤖 OBJECT bit: Yes, needed. Factors have a class attribute, and the OBJECT bit tells R "this is a classed object, use S3 method dispatch". Without it, things like print(), [, etc. might not dispatch to the factor methods correctly.

Either way, sounds like no impact.

thisisnic · 2025-12-23T08:09:30Z

r/src/altrep.cpp

+    return R_altrep_inherits(x, AltrepVectorPrimitive<REALSXP>::class_t) ||
+           R_altrep_inherits(x, AltrepVectorPrimitive<INTSXP>::class_t) ||
+           R_altrep_inherits(x, AltrepFactor::class_t) ||
+           R_altrep_inherits(x, AltrepVectorString<StringType>::class_t) ||
+           R_altrep_inherits(x, AltrepVectorString<LargeStringType>::class_t);


🤖 Trade-off: Old code automatically worked for any class registered under "arrow" package. New code requires updating if new ALTREP types are added.

Doesn't sound like it'll be an issue?

Maybe, but this check is definitely more fragile — do we have to do this here? I don't see any of the ATTRIB stuff here?

In the code I deleted from r/src/arrow_cpp11.h, ALTREP_CLASS_SERIALIZED_CLASS calls ATTRIB

thisisnic · 2025-12-23T08:10:46Z

r/src/altrep.cpp

+  if (R_altrep_inherits(x, arrow::r::altrep::AltrepVectorPrimitive<REALSXP>::class_t)) {
    result = arrow::r::altrep::AltrepVectorPrimitive<REALSXP>::IsMaterialized(x);
-  } else if (class_name == "arrow::array_int_vector") {
+  } else if (R_altrep_inherits(
+                 x, arrow::r::altrep::AltrepVectorPrimitive<INTSXP>::class_t)) {
    result = arrow::r::altrep::AltrepVectorPrimitive<INTSXP>::IsMaterialized(x);
-  } else if (class_name == "arrow::array_string_vector") {
+  } else if (R_altrep_inherits(
+                 x, arrow::r::altrep::AltrepVectorString<arrow::StringType>::class_t)) {
    result = arrow::r::altrep::AltrepVectorString<arrow::StringType>::IsMaterialized(x);
-  } else if (class_name == "arrow::array_large_string_vector") {
+  } else if (R_altrep_inherits(
+                 x,
+                 arrow::r::altrep::AltrepVectorString<arrow::LargeStringType>::class_t)) {
    result =
        arrow::r::altrep::AltrepVectorString<arrow::LargeStringType>::IsMaterialized(x);
-  } else if (class_name == "arrow::array_factor") {
+  } else if (R_altrep_inherits(x, arrow::r::altrep::AltrepFactor::class_t)) {
    result = arrow::r::altrep::AltrepFactor::IsMaterialized(x);


I checked we are checking the same thing before/after; appears that we are but just accessing them differently (i.e. via C++ class name not registered name)?

This new way sounds better to me, the class is a truer test than what we labelled it 😂

I haven't pulled the code locally — but do we use class_name elsewhere or was this the one place? Maybe we could get rid of class_name entirely if we always do something like ^^^^?

We have it here + also where we have Rprintf("materialized %s len=%ld\n", Impl::class_name,. Unsure if it's important - Claude thinks it's just for altrep debugging, and looks like that to me?

thisisnic · 2025-12-23T08:11:44Z

r/src/altrep.cpp

+  if (R_altrep_inherits(x, arrow::r::altrep::AltrepVectorPrimitive<REALSXP>::class_t)) {
    arrow::r::altrep::AltrepVectorPrimitive<REALSXP>::Materialize(x);
-  } else if (class_name == "arrow::array_int_vector") {
+  } else if (R_altrep_inherits(
+                 x, arrow::r::altrep::AltrepVectorPrimitive<INTSXP>::class_t)) {
    arrow::r::altrep::AltrepVectorPrimitive<INTSXP>::Materialize(x);
-  } else if (class_name == "arrow::array_string_vector") {
+  } else if (R_altrep_inherits(
+                 x, arrow::r::altrep::AltrepVectorString<arrow::StringType>::class_t)) {
    arrow::r::altrep::AltrepVectorString<arrow::StringType>::Materialize(x);
-  } else if (class_name == "arrow::array_large_string_vector") {
+  } else if (R_altrep_inherits(
+                 x,
+                 arrow::r::altrep::AltrepVectorString<arrow::LargeStringType>::class_t)) {
    arrow::r::altrep::AltrepVectorString<arrow::LargeStringType>::Materialize(x);
-  } else if (class_name == "arrow::array_factor") {
+  } else if (R_altrep_inherits(x, arrow::r::altrep::AltrepFactor::class_t)) {


See above comment (https://github.com/apache/arrow/pull/48634/files#r2642335344)

thisisnic · 2025-12-23T08:12:44Z

r/src/altrep.cpp


  // singleton altrep class description
  static R_altrep_class_t class_t;
+  static const char* class_name;


This felt like slightly weird/redundant weird way to get this information but didn't see any alternative approach.

This might be mostly my lack-of-C++ fluency but do you know why we need to have lines like this on a few lines (here and below)? What is it doing?

We need this to extract the class name a different way - before we were doing it via code which used ATTRIB - so having it declared here means that we can set it when we initialise the class (where we do AltrepClass::class_name = name;).

thisisnic · 2025-12-23T08:18:15Z

CI failure R / AMD64 Ubuntu 24.04 R 4.4 Force-Tests true (pull_request) due to unrelated issue (downloading Parquet file from our repo)

thisisnic · 2025-12-24T10:28:54Z

Alternative approach to try based on conversation with CRAN folks:

The other one is tricky (L152 , 1163, 1194) - you are trying to get the class name of the altrep object for which there is no API AFAICT. ...A workaround is to use something like:

const char *altrep_class_name(SEXP x) {
   SEXP l = PROTECT(Rf_lang2(Rf_install(".Internal"),
			  PROTECT(Rf_lang2(Rf_install("altrep_class"), x))));
   SEXP r = Rf_eval(l, R_BaseEnv);
   UNPROTECT(2);
   return CHAR(STRING_ELT(r, 0));
}

@jonkeane - I'm leaning towards not implementing this approach due to advice from 🤖

🤖 The above suggestion calls into R at runtime to query the class name, while the current PR simply stores the name as a static string when the class is registered. Since Arrow controls its own altrep classes and already has the name at registration time, storing it is simpler, faster, and avoids the overhead of R evaluation.

Remove non-API calls

084745d

github-actions bot added Component: R awaiting committer review Awaiting committer review labels Dec 23, 2025

thisisnic commented Dec 23, 2025

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Dec 23, 2025

thisisnic commented Dec 23, 2025

View reviewed changes

thisisnic marked this pull request as ready for review December 23, 2025 08:43

thisisnic requested a review from jonkeane as a code owner December 23, 2025 08:43

GH-48631: [R] Non-API calls: 'ATTRIB', 'SET_ATTRIB' #48634

Are you sure you want to change the base?

GH-48631: [R] Non-API calls: 'ATTRIB', 'SET_ATTRIB' #48634

Conversation

thisisnic commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Summary of AI changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic commented Dec 23, 2025

Uh oh!

thisisnic commented Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thisisnic commented Dec 23, 2025 •

edited

Loading

thisisnic Dec 24, 2025 •

edited

Loading

thisisnic Dec 23, 2025 •

edited

Loading

thisisnic Dec 24, 2025 •

edited

Loading

thisisnic commented Dec 24, 2025 •

edited

Loading