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.02%. Comparing base (b472d86) to head (b4f5702).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7487      +/-   ##
==========================================
+ Coverage   98.99%   99.02%   +0.03%     
==========================================
  Files          87       87              
  Lines       16729    16750      +21     
==========================================
+ Hits        16561    16587      +26     
+ Misses        168      163       -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 stopped early for fread(colClasses='Date') improved in #6107

Generated via commit 069560c

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

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

HughParsonage

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

src/assign.c

src/utils.c

src/dogroups.c

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

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

src/dogroups.c

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.

ben-schwen

LGTM, TY!
(Besides missing news ofc)

### 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](https://claude.com/claude-code) Have added comments inline regarding assumptions behind changes, and where I questioned those to try to verify it. Sources referenced in approach: - Rdatatable/data.table#7487 - similar fixes - https://cran.r-project.org/doc/manuals/r-release/R-ints.html on `DUPLICATE_ATTRIB` * GitHub Issue: #48631 Authored-by: Nic Crane <[email protected]> Signed-off-by: Nic Crane <[email protected]>

* frev: drop SET_ATTRIB Instead, backport and use CLEAR_ATTRIB (R >= 4.5). * mergeIndexAttrib: drop SET_ATTRIB 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 * nafillR: use ANY_ATTRIB * Backport R_mapAttrib * anySpecialStatic: switch to R_mapAttrib * dogroups: construct rownames anew 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 * assign: factor out index fixup 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. * assign: drop indexLength * assign: fix index unmarking * Comments, better field names * Update src/dogroups.c Co-authored-by: Benjamin Schwendinger <[email protected]> * mapAttrib: protect the attribute value 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 This solution is closer to the working approach previously taken by the code. * Fix comment, function name * Protect the newly found rownames attribute * add NEWS entry --------- Co-authored-by: HughParsonage <[email protected]> Co-authored-by: Benjamin Schwendinger <[email protected]> Co-authored-by: Michael Chirico <[email protected]> Co-authored-by: Michael Chirico <[email protected]>

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

src/assign.c Show resolved Hide resolved

src/assign.c Outdated Show resolved Hide resolved

src/assign.c Outdated Show resolved Hide resolved

src/assign.c Outdated Show resolved Hide resolved

src/utils.c Outdated Show resolved Hide resolved

src/dogroups.c Outdated Show resolved Hide resolved

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

Merged

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

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

Merged

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 4 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

Protect the newly found rownames attribute

b1daaab

ben-schwen approved these changes Jan 8, 2026

View reviewed changes

MichaelChirico and others added 2 commits January 8, 2026 22:47

Merge branch 'master' into attrib

069560c

add NEWS entry

b4f5702

MichaelChirico merged commit 4d30c7f into master Jan 9, 2026
10 of 11 checks passed

MichaelChirico deleted the attrib branch January 9, 2026 06:55

ben-schwen mentioned this pull request Jan 9, 2026

remove unused vars #7578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace `ATTRIB`, `SET_ATTRIB` #7487

Replace `ATTRIB`, `SET_ATTRIB` #7487

aitap commented Dec 19, 2025

Uh oh!

codecov bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

HughParsonage left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aitap commented Dec 25, 2025

Uh oh!

Uh oh!

ben-schwen Dec 26, 2025

Uh oh!

Uh oh!

ben-schwen left a comment •

edited

Loading

Uh oh!

ben-schwen left a comment •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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

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!

Uh oh!

Uh oh!

aitap commented Dec 25, 2025

Uh oh!

Uh oh!

ben-schwen Dec 26, 2025

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 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 left a comment •

edited

Loading

ben-schwen left a comment •

edited

Loading