Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aitap wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2bb8d34
copy(): reimplement in C
aitap Jul 18, 2026
b22a456
test 2224.3: replace pairlist columns with VECSXP
aitap Jul 18, 2026
992c71d
NEWS item
aitap Jul 18, 2026
e2a4655
Exercise list attributes
aitap Jul 18, 2026
cf6449a
Exercise data.tables as attributes
aitap Jul 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@

13. `rbindlist()` (and therefore the `rbind()` method for `data.table`s) no longer raises an error upon encountering more than approximately 50000 columns in a list entry, [#7793](https://github.com/Rdatatable/data.table/issues/7793). The bug was introduced in `data.table` version 1.18.2.1. Thanks to @rickhelmus for the report and @aitap for the fix.

14. `copy()` is now more consistent about reallocating nested `data.table`s, [#7456](https://github.com/Rdatatable/data.table/issues/7456). The resulting list is now only overwritten when necessary, and its attributes are walked in search of data.tables to reallocate as well. Thanks to @be-marc for the report, @david-cortes for additional information, and @aitap for the fix.

### Notes

1. {data.table} now depends on R 3.5.0 (2018).
Expand Down
22 changes: 2 additions & 20 deletions R/data.table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,7 +567,7 @@
stopf("When by and keyby are both provided, keyby must be TRUE or FALSE")
}
if (missing(by)) { missingby=TRUE; by=bysub=NULL } # possible when env is used, PR#4304
else if (verbose && !is.null(env)) catf("Argument '%s' after substitute: %s\n", "by", paste(deparse(bysub, width.cutoff=500L), collapse="\n"))

Check warning on line 570 in R/data.table.R

View workflow job for this annotation

GitHub Actions / lint-r 

file=R/data.table.R,line=570,col=91,[paste_linter] Use deparse1(x) instead of paste(deparse(x), collapse = ...).
}
bynull = !missingby && is.null(by) #3530
byjoin = !is.null(by) && is.symbol(bysub) && bysub==".EACHI"
Expand Down Expand Up @@ -632,7 +632,7 @@
substitute2(.j, env),
list(.j = substitute(j))
))
if (missing(jsub)) {j = substitute(); jsub=NULL} else if (verbose && !is.null(env)) catf("Argument '%s' after substitute: %s\n", "j", paste(deparse(jsub, width.cutoff=500L), collapse="\n"))

Check warning on line 635 in R/data.table.R

View workflow job for this annotation

GitHub Actions / lint-r 

file=R/data.table.R,line=635,col=141,[paste_linter] Use deparse1(x) instead of paste(deparse(x), collapse = ...).
}
}
if (!missing(j)) {
Expand Down Expand Up @@ -722,7 +722,7 @@
substitute2(.i, env),
list(.i = substitute(i))
))
if (missing(isub)) {i = substitute(); isub=NULL} else if (verbose && !is.null(env)) catf("Argument '%s' after substitute: %s\n", "i", paste(deparse(isub, width.cutoff=500L), collapse="\n"))

Check warning on line 725 in R/data.table.R

View workflow job for this annotation

GitHub Actions / lint-r 

file=R/data.table.R,line=725,col=141,[paste_linter] Use deparse1(x) instead of paste(deparse(x), collapse = ...).
}
}
if (!missing(i)) {
Expand Down Expand Up @@ -2252,7 +2252,7 @@

# What's the name of the top-level call in 'j'?
# NB: earlier, we used 'as.character()' but that fails for closures/builtins (#6026).
root_name = function(jsub) if (is.call(jsub)) paste(deparse(jsub[[1L]]), collapse = " ") else ""

Check warning on line 2255 in R/data.table.R

View workflow job for this annotation

GitHub Actions / lint-r 

file=R/data.table.R,line=2255,col=47,[paste_linter] Use deparse1(x) instead of paste(deparse(x), collapse = ...).

DT = function(x, ...) { #4872
old = getOption("datatable.optimize")
Expand Down Expand Up @@ -2780,26 +2780,8 @@

# TO DO, add more warnings e.g. for by.data.table(), telling user what the data.table syntax is but letting them dispatch to data.frame if they want

copy = function(x) {
newx = .Call(Ccopy,x) # copies at length but R's duplicate() also copies truelength over.
# TO DO: inside Ccopy it could reset tl to 0 or length, but no matter as selfrefok detects it
# TO DO: revisit duplicate.c in R 3.0.3 and see where it's at

reallocate = function(y) {
if (is.data.table(y)) {
.Call(C_unlock, y)
setalloccol(y)
} else if (is.list(y)) {
oldClass = class(y)
setattr(y, 'class', NULL) # otherwise [[.person method (which returns itself) results in infinite recursion, #4620
y[] = lapply(y, reallocate)
if (!identical(oldClass, 'list')) setattr(y, 'class', oldClass)
}
y
}

reallocate(newx)
}
copy = function(x)
.Call(Ccopy, x, getOption('datatable.alloccol'))

.shallow = function(x, cols = NULL, retain.key = FALSE, unlock = FALSE) {
wasnull = is.null(cols)
Expand Down
6 changes: 6 additions & 0 deletions inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21831,3 +21831,9 @@ test(2378.92, .Call(CresizeVector, x, 2:3), error = "must be length 1 non-NA non
test(2378.93, .Call(CresizeVector, x, -2L), error = "must be length 1 non-NA non-negative integer value")
test(2378.94, .Call(CresizeVector, x, NA_integer_), error = "must be length 1 non-NA non-negative integer value")
test(2378.95, .Call(CresizeVector, NULL, 2L), error = "must be a vector")

# copy() reallocates data.tables inside list attributes, #7820
x = list()
attr(x, 'dt') = data.table()
test(2379, truelength(attr(copy(x), 'dt')) > 0L)

@ben-schwen ben-schwen Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if we have multiple attributes like:

x = list()
attr(x, 'foo') = 1:2
attr(x, 'dt') = data.table()
truelength(attr(copy(x), 'dt'))

rm(x)

@ben-schwen ben-schwen Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we add a test for nested tables?

inner = data.table(a=1)
DT = data.table(x=1:2, y=list(inner, inner))
DT2 = copy(DT)
test(2379.1, truelength(DT2$y[[1L]]) > 0L)  # is FALSE in current master
test(2379.2, DT2$y[[1L]][, b := 2][, b], 2) # warns in current master

2 changes: 1 addition & 1 deletion src/data.table.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,7 @@ SEXP copyCols(SEXP x, SEXP cols);
// where there are no arguments, it must be (void) not () to be a strict prototype
SEXP setattrib(SEXP, SEXP, SEXP);
SEXP assign(SEXP, SEXP, SEXP, SEXP, SEXP);
SEXP copy(SEXP);
SEXP copy(SEXP, SEXP);
SEXP setdt_nrows(SEXP);
SEXP alloccolwrapper(SEXP, SEXP, SEXP);
SEXP allocrowwrapper(SEXP, SEXP);
Expand Down
2 changes: 1 addition & 1 deletion src/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ static const R_CallMethodDef callMethods[] = {
{"Cbmerge", (DL_FUNC)&bmerge, -1},
{"Cassign", (DL_FUNC)&assign, -1},
{"Cdogroups", (DL_FUNC)&dogroups, -1},
{"Ccopy", (DL_FUNC)&copy, -1},
{"Ccopy", (DL_FUNC)&copy, 2},
{"Cshallowwrapper", (DL_FUNC)&shallowwrapper, -1},
{"Csetdt_nrows", (DL_FUNC)&setdt_nrows, -1},
{"Calloccolwrapper", (DL_FUNC)&alloccolwrapper, -1},
Expand Down
45 changes: 42 additions & 3 deletions src/wrappers.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,48 @@ SEXP setlevels(SEXP x, SEXP levels, SEXP ulevels) {
return(x);
}

SEXP copy(SEXP x)
{
return(duplicate(x));
struct realloc_nested_dt_attr_ctx {
SEXP x;
int overAlloc;
};
static SEXP realloc_nested_dt_list(SEXP x, int overAlloc, bool);

@ben-schwen ben-schwen Jul 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static SEXP realloc_nested_dt_list(SEXP x, int overAlloc, bool);
static SEXP realloc_nested_dt_list(SEXP x, int overAlloc, bool replacePairlists);

or dont name arguments at all.

static SEXP realloc_nested_dt_list_attr(SEXP tag, SEXP att, void *ctx_) {
struct realloc_nested_dt_attr_ctx *ctx = ctx_;
SEXP newatt = realloc_nested_dt_list(att, ctx->overAlloc, false);
if (newatt != att) {
PROTECT(newatt);
setAttrib(ctx->x, tag, newatt);
UNPROTECT(1);
}
return R_NilValue;
}
static SEXP realloc_nested_dt_list(SEXP x, int overAlloc, bool replacePairlists) {
int nprot = 0;
if (inherits(x, "data.table")) {
x = PROTECT(alloccol(x, LENGTH(x) + overAlloc, FALSE)); ++nprot;
unlock(x);
}
if (replacePairlists && TYPEOF(x) == LISTSXP) {
x = PROTECT(coerceVector(x, VECSXP)); ++nprot;
}
if (TYPEOF(x) == VECSXP) for (R_xlen_t i = 0; i < XLENGTH(x); ++i) {
SEXP xi = VECTOR_ELT(x, i);
SEXP xinew = realloc_nested_dt_list(xi, overAlloc, false);
if (xinew != xi)
SET_VECTOR_ELT(x, i, xinew);
}
struct realloc_nested_dt_attr_ctx ctx = { .x = x, .overAlloc = overAlloc };
R_mapAttrib(x, realloc_nested_dt_list_attr, &ctx);
UNPROTECT(nprot);
return x;
}

SEXP copy(SEXP x, SEXP overAllocArg) {
int overAlloc = checkOverAlloc(overAllocArg);
PROTECT(x = duplicate(x));
x = realloc_nested_dt_list(x, overAlloc, true);
UNPROTECT(1);
return x;
}

// Internal use only. So that := can update elements of a list of data.table, #2204. Just needed to overallocate/grow the VECSXP.
Expand Down
Loading