-
Notifications
You must be signed in to change notification settings - Fork 990
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
Remove user-facing references to .internal.selfref #6698
base: master
Are you sure you want to change the base?
Conversation
Generated via commit 3e12c02 Download link for the artifact containing the test results: ↓ atime-results.zip
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6698 +/- ##
=======================================
Coverage 98.61% 98.61%
=======================================
Files 79 79
Lines 14553 14553
=======================================
Hits 14352 14352
Misses 201 201 ☔ View full report in Codecov by Sentry. |
@@ -1195,7 +1195,7 @@ replace_dot_alias = function(e) { | |||
# ok=-1 which will trigger setalloccol with verbose in the next | |||
# branch, which again calls _selfrefok and returns the message then | |||
if ((ok<-selfrefok(x, verbose=FALSE))==0L) # ok==0 so no warning when loaded from disk (-1) [-1 considered TRUE by R] | |||
if (is.data.table(x)) warningf("Invalid .internal.selfref detected and fixed by taking a (shallow) copy of the data.table so that := can add this new column by reference. At an earlier point, this data.table has been copied by R (or was created manually using structure() or similar). Avoid names<- and attr<- which in R currently (and oddly) may copy the whole data.table. Use set* syntax instead to avoid copying: ?set, ?setnames and ?setattr. If this message doesn't help, please report your use case to the data.table issue tracker so the root cause can be fixed or this message improved.") | |||
if (is.data.table(x)) warningf("A shallow copy of this data.table was taken so that := can add %d new columns by reference. At an earlier point, this data.table was copied by R (or was created manually using structure() or similar). Avoid names<- and attr<- which in R currently (and oddly) may copy the whole data.table. Use set* syntax instead to avoid copying: ?set, ?setnames and ?setattr. It's also common for data.table-unaware packages to produce affected tables. If this message doesn't help, please report your use case to the data.table issue tracker so the root cause can be fixed or this message improved.", length(newnames)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just found a new corner case when removing a non-existing column with invalid internal.selfref.
DT = data.frame(x=1L, y=1:3)
setattr(DT, 'class', c("data.table", "data.frame"))
DT[, z := NULL]
# Warning messages:
# 1: In `[.data.table`(DT, , `:=`(z, NULL)) :
# A shallow copy of this data.table was taken so that := can add 1 new columns by reference. At an earlier point, this data.table was copied by R (or was created manually using structure() or similar). Avoid names<- and attr<- which in R currently (and oddly) may copy the whole data.table. Use set* syntax instead to avoid copying: ?set, ?setnames and ?setattr. It's also common for data.table-unaware packages to produce affected tables. If this message doesn't help, please report your use case to the data.table issue tracker so the root cause can be fixed or this message improved.
# 2: In `[.data.table`(DT, , `:=`(z, NULL)) :
# Tried to assign NULL to column 'z', but this column does not exist to remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's under
or was created manually using structure() or similar
The wording looks OK to me, do you have a suggestion? Or maybe the trouble is it could look somewhat contradictory with the next sentence
Use set* syntax instead to avoid copying
Where setattr
was already used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just find it strange that we speak of add 1 new columns by reference
when we are in fact dropping a column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting corner case indeed. At this point newnames
is setdiff(lhs, names(x))
where lhs
is "z"
and names(x)
is (as expected) c("x", "y")
. The code doesn't realise yet that the non-existent z
is mentioned in the j
expression only to be deleted.
@@ -119,7 +119,7 @@ static int _selfrefok(SEXP x, Rboolean checkNames, Rboolean verbose) { | |||
} | |||
p = R_ExternalPtrAddr(v); | |||
if (p==NULL) { | |||
if (verbose) Rprintf(_(".internal.selfref ptr is NULL. This is expected and normal for a data.table loaded from disk. Please remember to always setDT() immediately after loading to prevent unexpected behavior. If this table was not loaded from disk or you've already run setDT(), please report to data.table issue tracker.\n")); | |||
if (verbose) Rprintf(_("This table is missing data.table internal attributes. This is expected and normal for a data.table loaded from disk. Please remember to always setDT() immediately after loading to prevent unexpected behavior. If this table was not loaded from disk or you've already run setDT(), please report to the data.table issue tracker.\n")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it matter that the attribute exists but is empty (not missing)? How about "The internal attributes of this data.table are empty."?
Closes #4519. Supersedes #6696.
Usage in internal errors is retained:
data.table/src/assign.c
Line 125 in 70c64ac
data.table/src/assign.c
Line 127 in 70c64ac
cc @aitap and @spiddy1204