Skip to content

copy(): only overwrite the list when necessary, consider the attributes#7820

Open
aitap wants to merge 5 commits into
masterfrom
fix7456
Open

copy(): only overwrite the list when necessary, consider the attributes#7820
aitap wants to merge 5 commits into
masterfrom
fix7456

Conversation

@aitap

@aitap aitap commented Jul 18, 2026

Copy link
Copy Markdown
Member

Rewrite copy() in C so that instead of calling y[] <- lapply(y, reallocate) we can consider each individual list element and only call SET_VECTOR_ELT when necessary. This avoids the problem with xgboost's special altlist that must never be altered.

Also walk the attributes of the object being copied, look for data.tables and reallocate them. Maybe that's not necessary (copy() has been working fine without it) and can be taken out.

Test 2224.3 points out an important detail: when y is a LISTSXP pairlist, y[] <- lapply(y, reallocate) makes it into a VECSXP list instead. It turns out we've been relying on copy() automatically coercing pairlist columns into vector lists when constructing data.tables. Should this become more explicit and be moved somewhere into the data.table() constructor, leaving copy() to just duplicate + reallocate data.tables?

Unfortunately this is hard to test without loading xgboost or creating a different altclass.

Fixes: #7456

aitap added 3 commits July 18, 2026 15:20
Don't overwrite list elements unless necessary. Also visit and adjust
lists inside attributes.
@aitap
aitap requested a review from MichaelChirico as a code owner July 18, 2026 15:29
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.02%. Comparing base (94649d5) to head (cf6449a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7820   +/-   ##
=======================================
  Coverage   99.01%   99.02%           
=======================================
  Files          88       88           
  Lines       17234    17250   +16     
=======================================
+ Hits        17065    17081   +16     
  Misses        169      169           

☔ View full report in Codecov by Harness.
📢 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

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown
  • HEAD=fix7456 much slower for transform improved in #5493
    Comparison Plot

Generated via commit cf6449a

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

Task Duration
R setup and installing dependencies 3 minutes and 27 seconds
Installing different package versions 24 seconds
Running and plotting the test cases 5 minutes and 29 seconds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Storing xgboost models in list columns fails

1 participant