Replace `ATTRIB`, `SET_ATTRIB` #7487

aitap · 2025-12-19T14:33:51Z

Towards #6180.

Replace remaining uses of SET_ATTRIB with SHALLOW_DUPLICATE_ATTRIB or CLEAR_ATTRIB. Replace remaining uses of ATTRIB testing with ANY_ATTRIB; use the new R_mapAttrib interface to walk the attributes.

The most invasive fix is in assign.c where attr(., 'index') is adjusted following a by-reference operation that could invalidate some indices. What used to be a for (cons = ATTRIB(x); !isNull(cons); cons = CDR(cons)) loop now has to construct a context struct to share between the caller and the map callback. On the plus side, I simplified the memory management and string operations a bit, getting rid of things like memset(string, '\0', 1) and making use of the new hash table to look up the indices.
The compact rownames attribute in dogroups.c is also a bit tricky. Now dogroups creates a throwaway INTSXP c(NA_integer, -whatever), gives it to setAttrib(SD, R_RowNamesSymbol, decoy), and obtains the actual compact rownames attribute that R re-creates from the return value. I think this is simpler than walking the attribute list by hand in search of the correct attribute, and the end result is the same: we can modify rownames by reference without spilling it into a large string vector.

Instead, backport and use CLEAR_ATTRIB (R >= 4.5).

Use SHALLOW_DUPLICATE_ATTRIB (R >= 3.3) for the simple case. Also, Backport ANY_ATTRIB (R >= 4.5) instead of testing !isNull(ATTRIB(.)).

Instead of trying to walk ATTRIB in search of the compact 'rownames' attribute to modify, install it anew, take note of the returned reference to the value being installed (a different one!) and modify that.

Instead of walking the attribute list directly, use R_mapAttrib(). Create a hash table of index names instead of relying on chin() and a temporary string vector. Move all temporary allocations onto the R heap.

codecov · 2025-12-19T14:40:17Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.07%. Comparing base (be67a4d) to head (2c10c37).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7487      +/-   ##
==========================================
+ Coverage   99.04%   99.07%   +0.03%     
==========================================
  Files          87       87              
  Lines       16671    16691      +20     
==========================================
+ Hits        16511    16536      +25     
+ Misses        160      155       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-12-19T14:55:58Z

HEAD=attrib slower P<0.001 for memrecycle regression fixed in #5463

Generated via commit 2c10c37

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	5 minutes and 8 seconds
Installing different package versions	11 minutes and 13 seconds
Running and plotting the test cases	5 minutes and 5 seconds

HughParsonage

Maybe add some brief documentation about what each function does and returns if time permits.

HughParsonage · 2025-12-21T05:33:47Z

src/assign.c

+  return NULL;
+}
+
+static SEXP fixIndexAttrib(SEXP tag, SEXP value, void *ctx_) {


Is SEXP the intended return type?

Yes, that's following the R_mapAttrib interface. I have to return (SEXP)NULL to continue iteration.

HughParsonage · 2025-12-21T05:38:23Z

src/assign.c

+struct attrib_name_ctx {
+  hashtab *indexNames;
+  SEXP index, assignedNames;
+  R_xlen_t len;


Can this field name be changed to make it clear what length it is the length of. R_xlen_t len_of<...>;

HughParsonage · 2025-12-21T06:04:40Z

src/assign.c

+      // indexLength > 0 indicates reordering. Drop it to avoid spurious reordering in non-indexed columns (#2372)
+      // shortened name already present indicates that index needs to be dropped to avoid duplicate indices.
+      setAttrib(index, tag, R_NilValue);
+      hash_set(indexNames, tag, 0);


Should this be?

hash_set(indexNames, PRINTNAME(tag), 0);

(tag is a SYMSXP, not a CHARSXP?)

HughParsonage · 2025-12-21T06:05:32Z

src/assign.c

  // newcolnames : add these columns (if any)
  // cols : column names or numbers corresponding to the values to set
  // rows : row numbers to assign
  R_len_t numToDo, targetlen, vlen, oldncol, oldtncol, coln, protecti=0, newcolnum, indexLength;


Is indexLength used in assign any more?

HughParsonage · 2025-12-21T06:07:01Z

src/utils.c

+
+  SEXP ret = NULL;
+  for (; !isNull(a); REPROTECT(a = CDR(a), i)) {
+    ret = fun(TAG(a), CAR(a), ctx);


Not blocking, but if time permits consider R's structure more closely with protection of the TAG(a) and CAR(a) in teh callback.

AFAIK while a is protected, TAG(a), CAR(a), and CDR(a) should also stay protected. What do you recommend to do instead?

HughParsonage · 2025-12-21T06:07:41Z

src/dogroups.c

+  PROTECT_WITH_INDEX(rownames = allocVector(INTSXP, 2), &rownamesi); nprotect++;
+  INTEGER(rownames)[0] = NA_INTEGER;
+  INTEGER(rownames)[1] = -maxGrpSize;
+  REPROTECT(rownames = setAttrib(SD, R_RowNamesSymbol, rownames), rownamesi);


I think this is wrong; check the return value of setAttrib

Consider R_mapAttrib for this.

R is somewhat inconsistent about this. setAttrib(foo, R_CommentSymbol, bar) and setAttrib(foo, R_ClassSymbol, bar) return R_NilValue, but generic installAttrib and our specific case row_names_gets do return the freshly installed value. Since it's not documented, R_mapAttrib might be a safer approach.

aitap · 2025-12-25T08:23:46Z

This is expected to become more of a problem around end of January.

aitap added 9 commits December 19, 2025 16:44

frev: drop SET_ATTRIB

0a6e0e7

Instead, backport and use CLEAR_ATTRIB (R >= 4.5).

mergeIndexAttrib: drop SET_ATTRIB

2e0c389

Use SHALLOW_DUPLICATE_ATTRIB (R >= 3.3) for the simple case. Also, Backport ANY_ATTRIB (R >= 4.5) instead of testing !isNull(ATTRIB(.)).

cbindlist: use ANY_ATTRIB

58d586b

nafillR: use ANY_ATTRIB

402f0d5

Backport R_mapAttrib

0f47f29

anySpecialStatic: switch to R_mapAttrib

ff280b5

dogroups: construct rownames anew

e4f1788

Instead of trying to walk ATTRIB in search of the compact 'rownames' attribute to modify, install it anew, take note of the returned reference to the value being installed (a different one!) and modify that.

mergeIndexAttrib: switch to R_mapAttrib

3c66757

assign: factor out index fixup

2c10c37

Instead of walking the attribute list directly, use R_mapAttrib(). Create a hash table of index names instead of relying on chin() and a temporary string vector. Move all temporary allocations onto the R heap.

aitap requested review from HughParsonage and MichaelChirico as code owners December 19, 2025 14:33

ben-schwen added this to the 1.19.0 milestone Dec 21, 2025

HughParsonage reviewed Dec 21, 2025

View reviewed changes

HughParsonage and others added 3 commits December 21, 2025 22:14

assign: drop indexLength

e1fa0a6

assign: fix index unmarking

09b1f7e

Comments, better field names

1a408bf

thisisnic mentioned this pull request Dec 23, 2025

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

Open

aitap modified the milestones: 1.19.0, 1.18.2 Dec 25, 2025

Replace ATTRIB, SET_ATTRIB #7487

Are you sure you want to change the base?

Replace ATTRIB, SET_ATTRIB #7487

Conversation

aitap commented Dec 19, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Dec 19, 2025

Uh oh!

HughParsonage left a comment

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!

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!

aitap commented Dec 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Replace `ATTRIB`, `SET_ATTRIB` #7487

Replace `ATTRIB`, `SET_ATTRIB` #7487

codecov bot commented Dec 19, 2025 •

edited

Loading