Skip to content

always unlock jval #3791

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

Merged
mattdowle merged 5 commits into from
Aug 27, 2019
Merged

always unlock jval #3791

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
2d9051f
j_lock_sd
Aug 27, 2019
b13fdb4
to pass tests used .Call(Csetattr,...) directly to avoid bumping MAYB…
mattdowle Aug 27, 2019
a6bf0d1
fix test number
Aug 27, 2019
b30e0cf
added alloc.col back to aid the PR comment then will remove again
mattdowle Aug 27, 2019
f6eda8c
removed temporary stop()
mattdowle Aug 27, 2019
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 @@ -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.
Expand Down
7 changes: 2 additions & 5 deletions R/data.table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand All @@ -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)) {
Expand Down
6 changes: 5 additions & 1 deletion inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 #
Expand Down
6 changes: 3 additions & 3 deletions src/assign.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]]
Expand Down