ARROW-15947: [R] rename_with s3 method for arrow_dplyr_query #12642
Conversation
|
|
|
Hi @MrMallIronmaker - thanks for submitting this PR! Unit tests are definitely the way to go in terms of next steps. Let us know if you have any questions about any of our set-up! |
|
I've written a few test cases for |
thisisnic
left a comment
thisisnic
left a comment
There was a problem hiding this comment.
Thanks for submitting this PR - it'll be great to have this functionality added!
A few minor comments and then also, please could you add an extra test where .cols doesn't end up selecting anything?
r/R/dplyr-select.R
Outdated
| new_names <- do.call(.fn, list(old_names)) | ||
| rename_args <- c(list(.data), as.list(old_names)) | ||
| names(rename_args) <- c(".data", new_names) | ||
| do.call(rename, rename_args) |
There was a problem hiding this comment.
| do.call(rename, rename_args) | |
| rename(.data, !!set_names(old_names, new_names)) |
Minor change here to make this more rlang style in keeping with the other code in the package + avoiding use of do.call().
r/R/dplyr-select.R
Outdated
| rename.Dataset <- rename.ArrowTabular <- rename.RecordBatchReader <- rename.arrow_dplyr_query | ||
|
|
||
| rename_with.arrow_dplyr_query <- function(.data, .fn, .cols = everything(), ...) { | ||
| .fn <- rlang::as_function(.fn) |
There was a problem hiding this comment.
In arrow/r/R/arrow-package-R we have a load of rlang function listed with @importFrom roxygen tags. Could you add as_function there and then call it directly here?
r/R/dplyr-select.R
Outdated
| rename_with.arrow_dplyr_query <- function(.data, .fn, .cols = everything(), ...) { | ||
| .fn <- rlang::as_function(.fn) | ||
| old_names <- names(select(.data, {{ .cols }})) | ||
| new_names <- do.call(.fn, list(old_names)) |
There was a problem hiding this comment.
Let's remove the do.call() here. Perhaps .fn(old_names) could work?
|
Changes made as requested - thanks! |
thisisnic
left a comment
thisisnic
left a comment
There was a problem hiding this comment.
Great - thanks for making those changes! One last tiny thing - the linter is complaining about spacing; would you mind running the styler on the files you've changes so that it automatically fixes those? More information on how to do that can be found here in the docs: https://arrow.apache.org/docs/r/articles/developers/workflow.html
|
I changed the spacing manually based on the lintr job output. |
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
This is looking great. Thank you for your contribution!
|
Benchmark runs are scheduled for baseline = ad7380e and contender = b781710. b781710 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
4 participants
Created a simple version of
rename_withthat applies the function to the current names of the .data argument and passes the result torename.This was a quick writeup on my end and passed a few smoke tests. I believe all the functions I've used in the new code are from base R or current imports like rlang, and the magic in editing the arrow data query is handled by
rename.Let me know what next steps can be, like writing test cases, etc.