Repository navigation
dplyr::arrange interfering with data.table's auto-indexing #5042
Description
Activity
could you report this with dplyr as well? at first glance it seems like a bug on that end
I think it might be related somewhat to #4889, and not fixed by #4893.
dplyr, AFAIK works with eithertibbles ordata.frames, so adata.table's index attribute would be ignored during that conversion but retained, leading to an incorrect result. I took a quick look atas.data.table.data.frame, which I believe would be called here, and it doesn't look like it removed the index:require(data.table) mtcars = as.data.table(mtcars) setindex(mtcars, mpg) mtcars2 = as.data.frame(mtcars) attributes(mtcars2)$indexThe last row returns an output:
integer(0) attr(,"__mpg") [1] 15 16 24 7 17 31 14 23 22 29 12 13 11 6 5 10 25 30 1 2 4 32 21 3 9 8 27 26 19 28 18 20which would remain unchanged in an operation such as
dplyr::arrangefollowed by aas.data.table. Source codes for easy reference:as.data.frame.data.table
function (x, ...) { ans = copy(x) setattr(ans, "row.names", .set_row_names(nrow(x))) setattr(ans, "class", "data.frame") setattr(ans, "sorted", NULL) setattr(ans, ".internal.selfref", NULL) ans } <bytecode: 0x00000256511304d0> <environment: namespace:data.table>as.data.table.data.frame
function (x, keep.rownames = FALSE, key = NULL, ...) { if (!identical(keep.rownames, FALSE)) { ans = data.table(rn = rownames(x), x, keep.rownames = FALSE, key = key) if (is.character(keep.rownames)) setnames(ans, "rn", keep.rownames[1L]) return(ans) } if (any(vapply_1i(x, function(xi) length(dim(xi))))) { return(as.data.table.list(as.list(x), keep.rownames = keep.rownames, ...)) } ans = copy(x) setattr(ans, "row.names", .set_row_names(nrow(x))) setattr(ans, "class", .resetclass(x, "data.frame")) setalloccol(ans) setkeyv(ans, key) ans } <bytecode: 0x0000025651853d60> <environment: namespace:data.table>Reacted by Jan Gorecki, Waldi73 and Matt DowleAs far as I can tell, dplyr explicitly preserves all attributes of the input. There needs to be a
dplyr_row_slice.data.tablefunction to accommodate the attributes that depend on row order, see
https://github.com/tidyverse/dplyr/blob/55917b6194aa8ba5b0ad8b2109ac746193996c96/R/generics.R#L25-L47Reacted by Cole MillerThanks all for analyzing this!
@MichaelChirico , reported on dplyr/issues/5917 as well@jangorecki I've tested your patch in the meantime and as I expected it does not solve the problem. One possibility (with probably minimal future maintainence, but that tends to be wishful thinking) would be to clear the attributes in
as.data.tableandsetDTbut I'm not really a fan of that.The following would fix the problem as well as obviating the need to call
as.data.tableat all here:dplyr_row_slice.data.table <- function(data, i, ...) { ans <- NextMethod() setattr(ans,"sorted",NULL) setattr(ans,"index",NULL) setalloccol(ans) return(ans) }
but that could start to get hairy as to who has to maintain it and beg the question of how much more should be done to make dplyr play nicely with data.table.
Reacted by Jan GoreckiWouldn't getting
as.data.tableandsetDTto recognize anindexattribute and updating it at the time of function call work that doesn't depend on other packages and have much maintenance overhead? I don't know much about the source code fordata.table, but I do remember reading discussions about a computationally efficient manner of checking if a vector is already ordered that is in use right now.A potential problem is if a different package or the user specifically creates an
indexattribute for adata.frameor derived object, that conflicts with howdata.tableuses that attribute - a chance that is much less likely.As @tlapak points out, dplyr defaults to preserving all attributes (since a reasonable number of people store arbitrary metadata in attributes and expect it to be maintained). As mentioned,
dplyr_row_slice.data.tablemethod would fix the problem, but it's unclear where it should live — since it affects the combination of dplyr and data.table, it feels odd to put it in either. I think it's slightly more correct for it to live in data.table (since y'all know best how to handle your own attributes), but would that require adding dplyr as a suggested package and a little work to conditionally register the method. Given that, I'd be happy to include the method in dplyr.@tlapak, you'd be welcome to do a PR if you wanted, and it would be great to get a sense from other data.table developers what attributes you think we should preserve/drop.
Reacted by Václav Tlapák and Waldi73Thanks all. #4893 now merged too and I confirmed it doesn't fix this one either, as @avimallu and @tlapak expected.
The root of this issue with
dplyr::arrangeappears to bevctrs::vec_slicewhichdplyr:::dplyr_row_slice.data.framecalls.> DT iso3c country income <char> <char> <char> 1: MOZ Mozambique LIC 2: ZMB Zambia LMIC 3: ALB Albania UMIC > attr(DT, "index") integer(0) attr(,"__iso3c") [1] 3 1 2 > > DT2 = vctrs::vec_slice(DT, c(3,1,2)) > > DT2 iso3c country income <char> <char> <char> 1: ALB Albania UMIC 2: MOZ Mozambique LIC 3: ZMB Zambia LMIC > attr(DT2, "index") integer(0) attr(,"__iso3c") [1] 3 1 2 # index left intact and now incorrect > class(DT2) [1] "data.table" "data.frame" # still a data.table
The intention is that
[.data.tabledetects thatdplyrandvctrsare notdata.table-aware, and anydata.frame-code (in this case[on adata.tablefrom those not-data-table-aware packages) gets redirected through thecedta()catch which drops the key and indices. That happens here :Line 153 in 6fa5cab
where thatsetkey(ans, NULL)removes the index attribute too as well the key. Test 304 was moved to be theplyr::arrangetest inother.Rrawwhich tests that :
data.table/inst/tests/other.Rraw
Line 59 in 0613157
But for
dplyr::arrange(dplyr not plyr),vctrs::vec_sliceis implemented in C :> vctrs::vec_slice function (x, i) { .Call(vctrs_slice, x, i) } <bytecode: 0x5617bbed46d8> <environment: namespace:vctrs>
Since
vctrs::vec_sliceis doing its own subsetting at C level, it's bypassing[method dispatch.There's been some attention here to
as.data.tableapparently because there's two calls toas.data.tablein the original reproducible example at the top :DT <- distinct(DT) %>% as.data.table() .... # Index mixed up by arrange DT <- DT %>% arrange(iso3c) %>% as.data.table()
But neither of those
as.data.table()are needed.dplyr::distinct(DT)anddplyr::arrange(DT)already retain thedata.tableclass so thoseas.data.table()calls just return their input. Hence, even if any potential fix was applied toas.data.table(for example like checking any index was valid and dropping it if not) wouldn't work ifas.data.table()isn't used.Even more minimal example :
require(data.table) require(dplyr) DT = data.table(A=c("b","c","a"), B=10:12) setindex(DT, A) DT[A=="c"] # correctly returns row 2 DT2 = dplyr::arrange(DT, A) class(DT2) # retains data.tabe class DT2[A=="c"] # incorrectly returns no rows
> sessionInfo() R version 4.1.0 (2021-05-18) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Pop!_OS 21.04 Matrix products: default BLAS: /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so.3 LAPACK: /usr/lib/x86_64-linux-gnu/openblas-pthread/libopenblasp-r0.3.13.so locale: [1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C [3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8 [5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8 [7] LC_PAPER=en_US.UTF-8 LC_NAME=C [9] LC_ADDRESS=C LC_TELEPHONE=C [11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] dplyr_1.0.6 data.table_1.14.1 loaded via a namespace (and not attached): [1] fansi_0.4.2 utf8_1.2.1 crayon_1.4.1 R6_2.5.0 [5] lifecycle_1.0.0 magrittr_2.0.1 pillar_1.6.0 rlang_0.4.11 [9] vctrs_0.3.8 generics_0.1.0 ellipsis_0.3.2 glue_1.4.2 [13] purrr_0.3.4 compiler_4.1.0 pkgconfig_2.0.3 tidyselect_1.1.1 [17] tibble_3.1.1The
.internal.selfrefis detecting the invalid status as intended :require(data.table) require(dplyr) DT = data.table(A=c("b","c","a"), B=10:12) setindex(DT, A) DT[A=="c"] # correctly returns row 2 data.table:::selfrefok(DT) # 1 i.e. ok DT2 = dplyr::arrange(DT, A) data.table:::selfrefok(DT2) # 0 i.e. not ok class(DT2) # retains data.tabe class DT2[A=="c", verbose=TRUE] # incorrectly returns no rows # ... # ... using index # ...
We shouldn't be using indexes when the selfref is not ok.
#5084 fixes this and adds a
dplyr::arrangetest totest.data.table("other.Rraw")But now we know what we're looking for, I suspect there are other entry points that need catching too.
setkey.R::getindex()could be a central place to drop index if selfref isn't ok. Would need to think of a test to trigger that without going throughbmerge()which is now caught by this fix toshallow().[.data.tablecould be another central place, at the top.Current thoughts are that it's probably best to merge #5084 so that folk can test dev, and then follow up in a new issue on a more comprehensive review adding tests for dropping indexes when selfref detects the prior copy by another package.
The advantage of this approach over adding
dplyr_row_slice.data.tableis that it tackles the root more directly and thus would fix other packages that may now or in future do the same thing asvctrs::vec_sliceat C level which bypasses[dispatch. The selfref mechanism was introduced to catch base R copies after all, so it's probably possible to reproduce an invalid index using base R (like the test in #4893) rather thandplyr::arrange(and if so #5084+#5085 would fix that too). We're just usingdplyr::arrangeas the first known example for the test really. There may be other methods indplyrthat find their way tovctrs::vec_slice, and maybe there are other functions invctrsthat work similarly tovec_slice, ordplyr's method names might change in future. All that should be covered by the selfref approach, so we insulate ourselves from maintenance, and users from specific version incompatibilities.That's the way I'm thinking currently, anyway. Happy to reconsider.
Reacted by Jan Gorecki and Václav Tlapák
Minimal reproducible exampleThis is a follow-up on this StackOverflow question/answer.
dplyr::arrangeseems to interfere with auto-indexing, leading to unexpected wrong results.A work-around is to disable auto-indexing with
options(datatable.auto.index = FALSE).As this issue is quite tricky, a warning, a mention to it in FAQ or a protection against this happening would be useful!
Output of sessionInfo()