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.00%. Comparing base (458d12a) to head (fa2df39).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7487      +/-   ##
==========================================
+ Coverage   98.97%   99.00%   +0.03%     
==========================================
  Files          87       87              
  Lines       16739    16759      +20     
==========================================
+ Hits        16567    16592      +25     
+ Misses        172      167       -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

No obvious timing issues in HEAD=attrib

Generated via commit fa2df39

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

Task	Duration
R setup and installing dependencies	3 minutes and 1 seconds
Installing different package versions	44 seconds
Running and plotting the test cases	5 minutes and 8 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?

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

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;

ben-schwen · 2025-12-26T16:30:18Z

src/dogroups.c

  // with PR#4164 started to copy input list columns too much. Hence PR#4655 in v1.13.2 moved that copy here just where it is needed.
  // Currently the marker is negative truelength. These specials are protected by us here and before we release them
  // we restore the true truelength for when R starts to use vector truelength.
  SEXP attribs, list_el;


Suggested change

SEXP list_el;

ben-schwen

Great work!

Minor nag that NEWS is missing.

Should we add an atime test like in #2389?

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

	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!

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!

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!

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!

Choose a reason for hiding this comment

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