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

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

mget bugs out .SD #1744

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


Copy link

@franknarf1 franknarf1 commented Jun 17, 2016

Here's what I see in 1.9.7:


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")

# "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.

Copy link

@eantonya eantonya commented Oct 21, 2016

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"

@eantonya eantonya reopened this Oct 21, 2016
@mattdowle mattdowle added this to the v1.9.10 milestone Nov 18, 2016
@mattdowle mattdowle removed this from the v1.9.8 milestone Nov 18, 2016
Copy link

@MichaelChirico 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
None yet

Successfully merging a pull request may close this issue.

5 participants