diff --git a/NEWS.md b/NEWS.md index bbcad04707..464f3cef35 100644 --- a/NEWS.md +++ b/NEWS.md @@ -236,6 +236,8 @@ 31. A `data.table` created using `setDT()` on a `data.frame` containing identical columns referencing each other would cause `setkey()` to return incorrect results, [#3496](https://github.com/Rdatatable/data.table/issues/3496) and [#3766](https://github.com/Rdatatable/data.table/issues/3766). Thanks @kirillmayantsev and @alex46015 for reporting, and @jaapwalhout and @Atrebas for helping to debug and isolate the issue. +32. `x[, round(.SD, 1)]` and similar operations on the whole of `.SD` could return a locked result, incorrectly preventing `:=` on the result, [#2245](https://github.com/Rdatatable/data.table/issues/2245). Thanks @grayskripko for raising. + #### NOTES 1. `rbindlist`'s `use.names="check"` now emits its message for automatic column names (`"V[0-9]+"`) too, [#3484](https://github.com/Rdatatable/data.table/pull/3484). See news item 5 of v1.12.2 below. diff --git a/R/data.table.R b/R/data.table.R index 3885eef2ed..ca5c88f6bd 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1265,6 +1265,8 @@ replace_order = function(isub, verbose, env) { } jval = eval(jsub, SDenv, parent.frame()) + .Call(Csetattrib, jval, '.data.table.locked', NULL) # in case jval inherits .SD's lock, #1341 #2245. Use .Call not setattr() to avoid bumping jval's MAYBE_SHARED. + # copy 'jval' when required # More speedup - only check + copy if irows is NULL # Temp fix for #921 - check address and copy *after* evaluating 'jval' @@ -1279,11 +1281,6 @@ replace_order = function(isub, verbose, env) { } else if (is.call(jsub) && jsub[[1L]] == "get") { jval = copy(jval) # fix for #1212 } - } else { - if (is.data.table(jval)) { - setattr(jval, '.data.table.locked', NULL) # fix for #1341 - if (!truelength(jval)) alloc.col(jval) - } } if (!is.null(lhs)) { diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index de3fe0282e..6b729c1161 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -15562,7 +15562,7 @@ test(2074.04, l[[c('foo', 'bar')]][ , (letters) := 16:18], error = 'under-alloca options(opt) ## alloc.col when using 0-truelength j assigning to a subset DT = data.table(a=1) -### construct incorrectly to have 0 truelength +### construct incorrectly to have 0 truelength; follow-up: https://github.com/Rdatatable/data.table/pull/3791#discussion_r318326736 zDT = structure(list(b=2), class = c('data.table', 'data.frame')) test(2074.05, DT[1L, b := zDT], data.table(a=1, b=2)) ## nested .SD in j @@ -15787,6 +15787,10 @@ setDT(x) test(2087.3, setkey(x, a, verbose=TRUE), data.table(a=paste0(1:2), b=paste0(1:2), c=paste0(1:2), key="a"), output='Found and copied 2 columns with a shared memory address') +# clear '.data.table.locked' even when is.null(irows), #2245 +x = data.table(a=c(0.85, -0.38, 1.19), b=c(0.56, 0.63, -1.30)) +test(2088, x[, round(.SD, 1)][, c:=8.88], data.table(a=c(.8, -.4, 1.2), b=c(.6,.6,-1.3), c=8.88)) + ################################### # Add new tests above this line # diff --git a/src/assign.c b/src/assign.c index ac56f17590..3838558d36 100644 --- a/src/assign.c +++ b/src/assign.c @@ -473,12 +473,12 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values) (TYPEOF(values)!=VECSXP && i>0) // assigning the same values to a second column. Have to ensure a copy #2540 ) { if (verbose) { - Rprintf("RHS for item %d has been duplicated because NAMED is %d, but then is being plonked. length(values)==%d; length(cols)==%d)\n", - i+1, NAMED(thisvalue), length(values), length(cols)); + Rprintf("RHS for item %d has been duplicated because NAMED==%d MAYBE_SHARED==%d, but then is being plonked. length(values)==%d; length(cols)==%d)\n", + i+1, NAMED(thisvalue), MAYBE_SHARED(thisvalue), length(values), length(cols)); } thisvalue = duplicate(thisvalue); // PROTECT not needed as assigned as element to protected list below. } else { - if (verbose) Rprintf("Direct plonk of unnamed RHS, no copy.\n"); // e.g. DT[,a:=as.character(a)] as tested by 754.3 + if (verbose) Rprintf("Direct plonk of unnamed RHS, no copy. NAMED==%d, MAYBE_SHARED==%d\n", NAMED(thisvalue), MAYBE_SHARED(thisvalue)); // e.g. DT[,a:=as.character(a)] as tested by 754.5 } SET_VECTOR_ELT(dt, coln, thisvalue); // plonk new column in as it's already the correct length setAttrib(thisvalue, R_NamesSymbol, R_NilValue); // clear names such as DT[,a:=mapvector[a]]