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 98.99%. Comparing base (458d12a) to head (012cf3a).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7487      +/-   ##
==========================================
+ Coverage   98.97%   98.99%   +0.01%     
==========================================
  Files          87       87              
  Lines       16739    16758      +19     
==========================================
+ Hits        16567    16589      +22     
+ Misses        172      169       -3

☔ 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

No obvious timing issues in HEAD=attrib

Generated via commit 012cf3a

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

Task	Duration
R setup and installing dependencies	2 minutes and 50 seconds
Installing different package versions	44 seconds
Running and plotting the test cases	5 minutes and 28 seconds

HughParsonage

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

src/assign.c

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?

any reason why we are not just backporting the implementation of R, since it seems to have the same level of complexity/speed

https://github.com/wch/r-source/blob/f21cd0f6e791dbe35292db51f96a25f46f306748/src/main/attrib.c#L2010-L2031

You've been right to tell me to consider protecting CAR(a). If the callback calls setAttrib and replaces the current attribute, CAR(a) becomes unprotected. TAG(a) is probably protected for symmetry, or to make the code future-proof against non-persistent symbols.

I'd like the function to be distinct from R's version because the compatibility page says that combining MPL (2.0) and GPL (>= 2) code results in a Larger Work subject to both licenses.

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.

ben-schwen · 2025-12-26T16:29:20Z

src/dogroups.c

+  SEXP rownames;
+  PROTECT_INDEX rownamesi;
+  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);


static SEXP findRowNames(SEXP key, SEXP val, void *data) { (void)data; if (key == R_RowNamesSymbol) return val; return NULL; }

SEXP rownames = R_mapAttrib(SD, findRowNames, NULL); if (rownames == NULL) error(_("row.names attribute of .SD not found")); if (!isInteger(rownames) || LENGTH(rownames)!=2 || INTEGER(rownames)[0]!=NA_INTEGER) error(_("row.names of .SD isn't integer length 2 with NA as first item; i.e., .set_row_names(). [%s %d %d]"),type2char(TYPEOF(rownames)),LENGTH(rownames),INTEGER(rownames)[0]);

ben-schwen · 2025-12-26T16:29:43Z

src/dogroups.c

  R_len_t ngrp, nrowgroups, njval=0, ngrpcols, ansloc=0, maxn, estn=-1, thisansloc, grpn, thislen, igrp;
  int nprotect=0;
-  SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, rownames, s, RHS, target, source;
+  SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, s, RHS, target, source;


Suggested change

SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, s, RHS, target, source;

SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, RHS, target, source;

src/dogroups.c

ben-schwen

Great work!

Minor nag that NEWS is missing.

Should we add an atime test like in #2389?

Co-authored-by: Benjamin Schwendinger <[email protected]>

Otherwise the callback could remove the attribute and end up with the value unprotected. Protect the attribute tag as well for uniformity. Co-Authored-By: HughParsonage <[email protected]>

This solution is closer to the working approach previously taken by the code.

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

Merge branch 'master' into attrib

fa2df39

ben-schwen reviewed Dec 26, 2025

View reviewed changes

src/dogroups.c Outdated Show resolved Hide resolved

ben-schwen reviewed Dec 26, 2025

View reviewed changes

ben-schwen mentioned this pull request Dec 28, 2025

Open up factor support in nafill #7539

Open

yutannihilation mentioned this pull request Dec 29, 2025

Remove ATTRIB() yutannihilation/savvy#420

Merged

Update src/dogroups.c

7381ec8

Co-authored-by: Benjamin Schwendinger <[email protected]>

aitap and others added 3 commits January 4, 2026 00:34

mapAttrib: protect the attribute value

987c613

Otherwise the callback could remove the attribute and end up with the value unprotected. Protect the attribute tag as well for uniformity. Co-Authored-By: HughParsonage <[email protected]>

dogroups: look up rownames using mapAttrib

ca0e85c

This solution is closer to the working approach previously taken by the code.

Fix comment, function name

012cf3a

	SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, s, RHS, target, source;
	SEXP ans=NULL, jval, thiscol, BY, N, I, GRP, iSD, xSD, RHS, target, source;

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HughParsonage left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben-schwen Dec 26, 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!

aitap commented Dec 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ben-schwen left a comment • 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!

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

github-actions bot commented Dec 19, 2025 •

edited

Loading

ben-schwen Dec 26, 2025 •

edited

Loading

ben-schwen left a comment •

edited

Loading