From 2d9051fa18c9ffba4cc528f3b764e0dc61e433c3 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Tue, 27 Aug 2019 16:19:41 +0800 Subject: [PATCH 1/4] j_lock_sd --- NEWS.md | 2 ++ R/data.table.R | 10 +++++----- inst/tests/tests.Rraw | 4 ++++ 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index 21ba02f96c..4059fa08f2 100644 --- a/NEWS.md +++ b/NEWS.md @@ -234,6 +234,8 @@ 30. `groupingsets` functions now properly handle alone special symbols when using an empty set to group by, [#3653](https://github.com/Rdatatable/data.table/issues/3653). Thanks to @Henrik-P for the report. +31. Some operations in `j` could leave the output with `.data.table.locked=TRUE`, preventing mutation of the table downstream, [#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 47c6729320..1640bf73c0 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1279,11 +1279,11 @@ replace_order = function(isub, verbose, env) { } else if (is.call(jsub) && jsub[[1L]] == "get" && is.list(jval)) { 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) - } + } + # #2245 jval can be data.table even if is.null(irows); unlock then as well + if (is.data.table(jval)) { + setattr(jval, '.data.table.locked', NULL) # fix for #1341 + if (!truelength(jval)) alloc.col(jval) } if (!is.null(lhs)) { # TODO?: use set() here now that it can add new columns. Then remove newnames and alloc logic above. diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index a84042353d..db32efcc1f 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -15771,6 +15771,10 @@ test(2086.04, DT[ , sum(a), keyby = list()], data.table(V1=55L)) test(2086.05, DT[ , sum(a), by = character()], data.table(V1=55L)) test(2086.06, DT[ , sum(a), keyby = character()], data.table(V1=55L)) +## #2245 unset .data.table.locked even if is.null(irows) +x <- data.table(a=c(0.85, -0.38, 1.19), b=c(0.56, 0.63, -1.30)) +test(2087, x[ , round(.SD, 1)][ , c := 8], data.table(a=c(.8, -.4, 1.2), b=c(.6,.6,-1.3), c=8)) + ################################### # Add new tests above this line # From a6bf0d1bc6e1c30957edd66f5b5c3a01a042b2b5 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Wed, 28 Aug 2019 06:24:05 +0800 Subject: [PATCH 2/4] fix test number --- src/assign.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/assign.c b/src/assign.c index d3b0a9ed56..3838558d36 100644 --- a/src/assign.c +++ b/src/assign.c @@ -478,7 +478,7 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values) } 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. NAMED==%d, MAYBE_SHARED==%d\n", NAMED(thisvalue), MAYBE_SHARED(thisvalue)); // 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]] From b30e0cfe3b987957dfbd909826e9921fb69aebbe Mon Sep 17 00:00:00 2001 From: mattdowle Date: Tue, 27 Aug 2019 15:34:27 -0700 Subject: [PATCH 3/4] added alloc.col back to aid the PR comment then will remove again --- R/data.table.R | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/R/data.table.R b/R/data.table.R index 458d66d3f5..6912d18c34 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1265,7 +1265,7 @@ 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_REFERENCED. + .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 @@ -1281,6 +1281,12 @@ 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) && !truelength(jval)) { + stop("just test 2074.05 (coverage) should touch this branch") + # will remove on next commit + alloc.col(jval) + } } if (!is.null(lhs)) { From f6eda8c7572001c2ad808d37288aee4a6fddf576 Mon Sep 17 00:00:00 2001 From: mattdowle Date: Tue, 27 Aug 2019 15:49:28 -0700 Subject: [PATCH 4/4] removed temporary stop() --- R/data.table.R | 6 ------ inst/tests/tests.Rraw | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/R/data.table.R b/R/data.table.R index 6912d18c34..ca5c88f6bd 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1281,12 +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) && !truelength(jval)) { - stop("just test 2074.05 (coverage) should touch this branch") - # will remove on next commit - alloc.col(jval) - } } if (!is.null(lhs)) { diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index 8f545003ea..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