From cbf13f112653f173f434ca456dc2163005eaaf44 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Fri, 8 Sep 2017 11:02:27 +0800 Subject: [PATCH] Fix a regression introduced in #2329. Closes #2338. --- NEWS.md | 2 +- R/data.table.R | 16 +++++++++------- inst/tests/tests.Rraw | 6 ++++++ 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/NEWS.md b/NEWS.md index 39b680c321..fd36d7723b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -76,7 +76,7 @@ 16. `split.data.table` respects `factor` ordering in `by` argument, [#2082](https://github.com/Rdatatable/data.table/issues/2082). Thanks to @MichaelChirico for identifying and fixing the issue. -17. `.SD` would incorrectly include symbol on lhs of `:=` when `.SDcols` is specified and `get()` appears in `j`. Thanks @renkun-ken for reporting and the PR. Closes #2326. +17. `.SD` would incorrectly include symbol on lhs of `:=` when `.SDcols` is specified and `get()` appears in `j`. Thanks @renkun-ken for reporting and the PR, and @ProfFancyPants for reporing a regression introduced in the PR. Closes [#2326](https://github.com/Rdatatable/data.table/issues/2336) and [#2338](https://github.com/Rdatatable/data.table/issues/2338). #### NOTES diff --git a/R/data.table.R b/R/data.table.R index 642baed5cf..405aeaaae5 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1051,13 +1051,7 @@ chmatch2 <- function(x, table, nomatch=NA_integer_) { } # fix for long standing FR/bug, #495 and #484 allcols = c(names(x), xdotprefix, names(i), idotprefix) - - # Do not include z in othervars when j is z := expr (#2326) - av_allcols <- intersect(av, allcols) - if (is.call(jsub) && length(jsub[[1L]]) == 1L && jsub[[1L]] == ":=" && is.symbol(jsub[[2L]])) { - av_allcols <- setdiff(av_allcols, as.character(jsub[[2L]])) - } - if ( length(othervars <- setdiff(av_allcols, c(bynames, ansvars))) ) { + if ( length(othervars <- setdiff(intersect(av, allcols), c(bynames, ansvars))) ) { # we've a situation like DT[, c(sum(V1), lapply(.SD, mean)), by=., .SDcols=...] or # DT[, lapply(.SD, function(x) x *v1), by=, .SDcols=...] etc., ansvars = union(ansvars, othervars) @@ -1085,6 +1079,14 @@ chmatch2 <- function(x, table, nomatch=NA_integer_) { # get('varname') is too difficult to detect which columns are used in general # eval(macro) column names are detected via the if jsub[[1]]==eval switch earlier above. } + + # Do not include z in .SD when dt[, z := {.SD; get("x")}, .SDcols = "y"] (#2326, #2338) + if (is.call(jsub) && length(jsub[[1L]]) == 1L && jsub[[1L]] == ":=" && is.symbol(jsub[[2L]])) { + jsub_lhs_symbol <- as.character(jsub[[2L]]) + if (jsub_lhs_symbol %chin% othervars) { + ansvars <- setdiff(ansvars, jsub_lhs_symbol) + } + } if (length(ansvars)) othervars = ansvars # #1744 fix allcols = c(names(x), xdotprefix, names(i), idotprefix) ansvars = setdiff(allcols,bynames) # fix for bug #5443 diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index 5dfd0534b7..9e20b7efc4 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -10494,6 +10494,12 @@ DT[, z := ncol(.SD) + get("y"), .SDcols = "x"] test(1823.2, DT$z, seq(11, 2)) DT[, z := ncol(.SD) + get("y"), .SDcols = c("x", "z")] test(1823.3, DT$z, seq(12, 3)) +DT[, x := {.SD; x + 1}, .SDcols = "y"] +test(1823.4, DT$x, seq(2, 11)) +DT[, z := ncol(.SD) + x, .SDcols = "y"] +test(1823.5, DT$z, seq(3, 12)) +DT[, z := ncol(.SD) + x, .SDcols = c("x", "y")] +test(1823.6, DT$z, seq(4, 13)) ##########################