Skip to content

fix names_sd rev dep issue 6033 #6238

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
MichaelChirico merged 6 commits into from
Jul 13, 2024
Merged

fix names_sd rev dep issue 6033 #6238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
89a22b3
fix names_sd rev dev issue 6033
ColeMiller1 Jul 11, 2024
ba6b70e
Simplify test and improve comment about version
ColeMiller1 Jul 11, 2024
d6d3d84
remove trailing ws
ben-schwen Jul 11, 2024
367d567
Merge branch 'master' into names_sd_revdep_followup
ColeMiller1 Jul 12, 2024
1024792
revdev->revdep
MichaelChirico Jul 12, 2024
28b0890
Merge branch 'master' into names_sd_revdep_followup
MichaelChirico Jul 13, 2024
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
5 changes: 5 additions & 0 deletions R/data.table.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1121,6 +1121,11 @@ replace_dot_alias = function(e) {
if (is.name(lhs)) {
lhs = as.character(lhs)
} else {
#6033 revdep. Slowly deprecate in 1.17.0. Caller has given us `dt[, substitute(names(.SD))]` which means
# jsub is actually substitute(names(.SD)) instead of just names(.SD)
if (lhs %iscall% 'substitute')
lhs = eval(lhs, parent.frame(), parent.frame())
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to double-eval()? Or can we do if (lhs %iscall% 'substitute') lhs = eval() else lhs = eval()?

Copy link
Copy Markdown
Contributor Author

@ColeMiller1 ColeMiller1 Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my understanding which is why I never understood how it used to work.

my_col = "a"
jsub = substitute(substitute(my_col))
eval(jsub, parent.frame(), parent.frame())
#  my_col
eval(eval(jsub))
# [1] "a"

At the same time, there's this head scratcher:

eval(jsub, list(my_col = "b"), parent.frame())
# [1] "b"

Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Head scratcher indeed... OK I'm more of the opinion we shouldn't allow j=substitute(...), indeed it's hard to reason about what jsub = substitute(subsitute(...)) might do. We can start a deprecation process & see if there's a good use case, my guess is, the end user would be better served by some other approach though.


# lhs is e.g. (MyVar) or get("MyVar") or names(.SD) || setdiff(names(.SD), cols)
lhs = eval(lhs, list(.SD = setNames(logical(length(sdvars)), sdvars)), parent.frame())
}
Expand Down
5 changes: 4 additions & 1 deletion inst/tests/tests.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18383,10 +18383,13 @@ test(2250.11, dt[, names(.SD(2)) := lapply(.SD, .I)], error=base_messages$missin
dt = data.table(a = 1:3, b = 5:7, grp = c('a', 'a', 'b'))
test(2250.12, dt[, names(.SD) := lapply(.SD, function(x) x + b), .SDcols = "a"], data.table(a = 1:3 + 5:7, b = 5:7, grp = c('a', 'a', 'b')))


dt = data.table(a = 1L, b = 2L, c = 3L, d = 4L, e = 5L, f = 6L)
test(2250.13, dt[, names(.SD)[1:5] := sum(.SD)], data.table(a = 21L, b = 21L, c = 21L, d = 21L, e = 21L, f = 6L))

dt = data.table(a = 1) #6033 revdep follow-up
Comment thread
MichaelChirico marked this conversation as resolved.
Outdated
my_col = "a"
test(2250.14, dt[, substitute(my_col) := .(3)], data.table(a = 3))

# fread(...,fill) can also be used to specify a guess on the maximum number of columns #2691 #1812 #4130 #3436 #2727
dt_str = paste(rep(c("1,2\n", "1,2,3\n"), each=100), collapse="")
ans = data.table(1L, 2L, rep(c(NA, 3L), each=100L))
Expand Down