-
Notifications
You must be signed in to change notification settings - Fork 1k
Open
Labels
Description
While experimenting with a potential fix for #5286 this test of DT surfaced:
D = copy(mtcars)
...
test(2212.02, EVAL("D |> DT(,.SD)"), mtcars)This line in [.data.table :
if (!is.data.table(ans)) setattr(ans, "class", c("data.table","data.frame")) # DF |> DT(,.SD[...]) .SD should be data.table, test 2212.013Seems to indicate that the desired behavior is to return a data.table. However ans isn't the variable that is returned directly, later on the code restores the class to data.frame before returning:
The returned object has class data.frame but still has data.table attributes (namely .internal.selfref).
Before suggesting fixes I believe some design decisions need to be explicitly discussed:
- Currently
DToperates freely on non-data-tables. The only discussion I could find of it is this single comment, and I'm not sure this is the best design.[.data.table, as the name implies, is a method of data.table. It might accidently work in some (even most) scenarios on data.frames, but is a hefty piece of code that was designed to use data.table attributes and trying to open it would mean an avalanche of bugs and a world of pain. In general, as a user I don't expect methods of one class to operate on another - on the contrary, I expect the class system to help me by isolating their respective functionalities.
In particular, rejecting non-data.table's would makeshallowadds .internal.selfref to an input data.frame #5286 (and probably others, both current and future) moot. - Inside
[.data.tableand other package methods I'd suggest usingsetDForsetDTinstead of settingclassdirectly - assetDFcleans up data.table-only attributes, that manual-class-set misses.
MichaelChirico and cthombor