[GitHub] [arrow] paleolimbot commented on a diff in pull request #13160: ARROW-14575: [R] Allow functions with {{pkg::}} prefixes

[GitBox <gi...@apache.org>]

paleolimbot commented on code in PR #13160:
URL: https://github.com/apache/arrow/pull/13160#discussion_r874274771


##########
r/R/dplyr-mutate.R:
##########
@@ -25,6 +25,12 @@ mutate.arrow_dplyr_query <- function(.data,
                                      .after = NULL) {
   call <- match.call()
   exprs <- ensure_named_exprs(quos(...))
+  exprs2 <- exprs
+  # replace `::` with `_` in passed expressions
+  for (i in seq_along(exprs)) {
+    exprs[[i]][[2]] <- gsub("::", "_", rlang::expr_text(exprs[[i]][[2]]))
+    exprs[[i]][[2]] <- parse_expr(exprs[[i]][[2]])
+  }

Review Comment:
   Rather than syntax transformation, I think it would be cleaner to register a binding for `::` and use it:
   
   ``` r
   library(arrow, warn.conflicts = FALSE)
   library(dplyr, warn.conflicts = FALSE)
   
   # :: prefixes because of the current namespace-stripping behaviour
   arrow:::register_binding("::somepkg::some_fun", function(x) x * 2)
   arrow:::register_binding("::::", function(lhs, rhs) {
     lhs_name <- as.character(substitute(lhs))
     rhs_name <- as.character(substitute(rhs))
     arrow:::nse_funcs[[paste0(lhs_name, "::", rhs_name)]]
   })
   arrow:::create_binding_cache()
   
   arrow_table(a = 1) %>% mutate(b = somepkg::some_fun(a))
   #> Table (query)
   #> a: double
   #> b: double (multiply_checked(a, 2))
   #> 
   #> See $.data for the source Arrow object
   ```
   
   ...but make sure to check that this doesn't give absolutely garbage error messages like `Error in (function(...) {...})(arg1, arg2)` because of the inlined function.
   
   



##########
r/R/dplyr-funcs.R:
##########
@@ -58,15 +58,16 @@ NULL
 #' @keywords internal
 #'
 register_binding <- function(fun_name, fun, registry = nse_funcs) {
-  name <- gsub("^.*?::", "", fun_name)
+  name <- gsub("^.*?_", "", fun_name)
   namespace <- gsub("::.*$", "", fun_name)

Review Comment:
   When I added that I used the syntax of `vctrs::s3_register()`, which expects `"package::function"` as it's function argument:
   
   https://github.com/r-lib/vctrs/blob/e86bb9a4b57459eb56099c0b52af37877a9bf515/R/register-s3.R#L52-L55
   
   I'd anticipated that a separate PR would make the change from (e.g.) `register_binding("as_datetime", ...)` to `register_binding("lubridate::as_datetime", ...)`, and perhaps one like this would use the `namespace` variable. I'd envisioned something more complicated but I really like your idea of registering twice, which is much simpler. With the `::` separator it's a bit more robust, since nobody is going to type:
   
   ```r
   table %>% mutate(`lubridate::as_datetime`(something))
   ```
   
   ...by accident, but they *might* type `table %>% mutate(lubridate_as_datetime(something))` (perhaps not with lubridate, but with a shorter package name you could probably get a clash pretty easily, perhaps even between registered function names).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org