Skip to content
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.

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

mget bugs out .SD #1744

Closed
franknarf1 opened this issue Jun 17, 2016 · 2 comments · Fixed by #3771
Closed

mget bugs out .SD #1744

franknarf1 opened this issue Jun 17, 2016 · 2 comments · Fixed by #3771
Labels
Milestone

Comments

@franknarf1
Copy link
Contributor

Here's what I see in 1.9.7:

library(data.table)

DT = data.table(ID = 1:2, A = 3:4, B = 5:6)
DT[, mget("A"), .SDcols="B"]            # ok
DT[, c(list(A=A), .SD), .SDcols="B"]    # ok
DT[, c(mget("A"), .SD), .SDcols="B"]    # bonkers, see below

#    A ID A B x.ID x.A x.B
#1: 3  1 3 5    1   3   5
#2: 4  2 4 6    2   4   6

DT[, {
    myA = mget("A")
    names(.SD)
}]

# "ID"   "A"    "B"    "x.ID" "x.A"  "x.B" 

It looks like .SD becomes strange after mget is called. This looks related to #994 which was addressed in 1.9.6 according to the NEWS.

@arunsrinivasan arunsrinivasan self-assigned this Jul 20, 2016
@arunsrinivasan arunsrinivasan added this to the v1.9.8 milestone Jul 20, 2016
@eantonya
Copy link
Contributor

I don't think this is fully fixed:

dt = data.table(a = 1, b = 2, c = 3)

dt[, {list(a, get('b')); names(.SD)}, .SDcols = 'c']
#[1] "a" "c"

@MichaelChirico
Copy link
Member

MichaelChirico commented Dec 21, 2016

DT[, c(mget("A"), .SD), .SDcols="B", verbose = TRUE]

'(m)get' found in j. ansvars being set to all columns. Use .SDcols or a single j=eval(macro) instead. Both will detect the columns used which is important for efficiency.
Old: B
New: ID,A,B

#    A B
# 1: 3 5
# 2: 4 6

So I guess the key is that the step where mget causes ansvars to be set to all columns is ignoring that .SDcols is actually specified. At a minimum, the warning message in verbose is incorrect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants