From a0d0f0ea61b1ff0f36959fbd5228d2bd5331a206 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 4 Jan 2024 08:21:24 +0000 Subject: [PATCH 1/4] fix issue with .SDcols using binary "-" --- R/data.table.R | 2 +- inst/tests/tests.Rraw | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/R/data.table.R b/R/data.table.R index 130543cbbc..284978b13f 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1008,7 +1008,7 @@ replace_dot_alias = function(e) { # peel from parentheses before negation so (-1L) works as well: as.data.table(as.list(1:3))[, .SD,.SDcols=(-1L)] #4231 while(colsub %iscall% "(") colsub = as.list(colsub)[[-1L]] # fix for R-Forge #5190. colsub[[1L]] gave error when it's a symbol. - if (colsub %iscall% c("!", "-")) { + if (colsub %iscall% "!" || (colsub %iscall% "-" && length(colsub) == 2L)) { negate_sdcols = TRUE colsub = colsub[[2L]] } else negate_sdcols = FALSE diff --git a/inst/tests/tests.Rraw b/inst/tests/tests.Rraw index 500aa30a97..f27abfcc0c 100644 --- a/inst/tests/tests.Rraw +++ b/inst/tests/tests.Rraw @@ -18179,3 +18179,8 @@ DT = data.table(id=1:2, x=1:2) r = copy(DT)[1L, x:= 5L] test(2241.13, DT, data.table(id=1:2, x=1:2)) test(2241.14, r, data.table(id=1:2, x=c(5L,2L))) + +# .SDcols using binary '-' is evaluated correctly (#5826 exposed this) +DT = data.table(a=1, b=2, c=3) +test(2242.1, DT[, .SD, .SDcols=2-1], DT[, .(a)]) +test(2242.2, DT[, .SD, .SDcols=length(DT)-1], DT[, .SD, .SDcols=2]) From c3ab4639351653af71fd3325d7a9405ca373de9e Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 4 Jan 2024 08:22:55 +0000 Subject: [PATCH 2/4] NEWS --- NEWS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/NEWS.md b/NEWS.md index eed4b7899b..4f521bdd2f 100644 --- a/NEWS.md +++ b/NEWS.md @@ -341,7 +341,7 @@ 23. `fread(fill=TRUE, verbose=TRUE)` would segfault on the out-of-sample type bump verbose output if the input did not contain column names, [5046](https://github.com/Rdatatable/data.table/pull/5046). Thanks to Václav Tlapák for the PR. -24. `.SDcols=-V2:-V1` and `.SDcols=(-1)` could error with `xcolAns does not pass checks` and `argument specifying columns specify non existing column(s)`, [#4231](https://github.com/Rdatatable/data.table/issues/4231). Thanks to Jan Gorecki for reporting and the PR. +24. `.SDcols=-V2:-V1` and `.SDcols=(-1)` could error with `xcolAns does not pass checks` and `argument specifying columns specify non existing column(s)`, [#4231](https://github.com/Rdatatable/data.table/issues/4231). Thanks to Jan Gorecki for reporting and the PR. Thanks Toby Dylan Hocking for tracking down an error caused by the initial fix and Michael Chirico for fixing it. 25. `.SDcols=` is now documented in `?data.table` and it is now an error if the logical vector's length is not equal to the number of columns (consistent with `data.table`'s no-recycling policy; see new feature 1 in v1.12.2 Apr 2019), [#4115](https://github.com/Rdatatable/data.table/issues/4115). Thanks to @Henrik-P for reporting and Jan Gorecki for the PR. From 5d1e905ddefcfeea666253ad9502c5c13ccaf7c9 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 4 Jan 2024 08:24:16 +0000 Subject: [PATCH 3/4] comment for emphasis --- R/data.table.R | 1 + 1 file changed, 1 insertion(+) diff --git a/R/data.table.R b/R/data.table.R index 284978b13f..7e5c56afbf 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1008,6 +1008,7 @@ replace_dot_alias = function(e) { # peel from parentheses before negation so (-1L) works as well: as.data.table(as.list(1:3))[, .SD,.SDcols=(-1L)] #4231 while(colsub %iscall% "(") colsub = as.list(colsub)[[-1L]] # fix for R-Forge #5190. colsub[[1L]] gave error when it's a symbol. + # NB: _unary_ '-', not _binary_ '-' (#5826) if (colsub %iscall% "!" || (colsub %iscall% "-" && length(colsub) == 2L)) { negate_sdcols = TRUE colsub = colsub[[2L]] From b506c3f392396db43173d94191a6d4a994351774 Mon Sep 17 00:00:00 2001 From: Michael Chirico Date: Thu, 4 Jan 2024 08:28:25 +0000 Subject: [PATCH 4/4] simplify/match equivalent code with jsub --- R/data.table.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/data.table.R b/R/data.table.R index 7e5c56afbf..76e6c4e0e0 100644 --- a/R/data.table.R +++ b/R/data.table.R @@ -1008,8 +1008,8 @@ replace_dot_alias = function(e) { # peel from parentheses before negation so (-1L) works as well: as.data.table(as.list(1:3))[, .SD,.SDcols=(-1L)] #4231 while(colsub %iscall% "(") colsub = as.list(colsub)[[-1L]] # fix for R-Forge #5190. colsub[[1L]] gave error when it's a symbol. - # NB: _unary_ '-', not _binary_ '-' (#5826) - if (colsub %iscall% "!" || (colsub %iscall% "-" && length(colsub) == 2L)) { + # NB: _unary_ '-', not _binary_ '-' (#5826). Test for '!' length-2 should be redundant but low-cost & keeps code concise. + if (colsub %iscall% c("!", "-") && length(colsub) == 2L) { negate_sdcols = TRUE colsub = colsub[[2L]] } else negate_sdcols = FALSE