Fast `select()` - Githubissues

tidyverse / dtplyr

Data table backend for dplyr

https://dtplyr.tidyverse.org

Other

670 stars 57 forks source link

Fast `select()` #375

Closed markfairbanks closed 2 years ago

markfairbanks commented 2 years ago

Closes #367

Overview:

Drop columns by reference in select() by using remove_vars() when:
- Data has been copied (implicitly or explicitly)
- There is no renaming in the select statement
- The selection can't be combined with a prior i step. Ex: dt[x < 7, .(x, y)]
Remove usage of remove_vars() from other functions and use select() for consistency

markfairbanks commented 2 years ago

Note: I'll update this to use the is_copied() util once #374 is merged

markfairbanks commented 2 years ago

@eutwt I'm in the process of adding tests. I see one of the tests uses skip_if(utils::packageVersion("rlang") < "0.5.0") - is there a reason we would need this? Both dplyr and dtplyr depend on rlang >= 1.0.0.

eutwt commented 2 years ago

Welp. I thought the two commits above would fix the snapshot errors. Sorry about that. I guess I can revert those two commits and then just edit the snapshot to match the "chr_as_locations" error. But I don't understand why it would have that in the error message, it looks to be using the latest version of rlang, vctrs, and testthat.

eutwt commented 2 years ago

I guess using error_call = .call in eval_select didn't work because that was only added in the development version, which I now see in the tidyselect news.md. Sorry for crudding up the PR with the commits.

markfairbanks commented 2 years ago

I guess using error_call = .call in eval_select didn't work because that was only added in the development version, which I now see in the tidyselect news.md. Sorry for crudding up the PR with the commits.

No worries, worth a try. Btw it looks like eval_select(error_call = ) exists in the CRAN version. Not sure what was causing the error in the checks. I'm going to merge this for now - maybe we can give it a try in another PR.