Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[R] Create a function registry for our NSE funcs #30529

Closed
asfimport opened this issue Dec 7, 2021 · 2 comments
Closed

[R] Create a function registry for our NSE funcs #30529

asfimport opened this issue Dec 7, 2021 · 2 comments

Comments

@asfimport
Copy link
Collaborator

asfimport commented Dec 7, 2021

This came up in the comments of ARROW-14575, but would be helpful even without doing anything with the namespace: instead of nse_funcs <- ..., create a registry + function to register each function and then use that registry in place of nse_funcs.

This will:

  • Give us a bit more freedom for defining bindings in separate files (though which nse_funcs+collate we could do the same). E.g. have one file per package
  • Possibly give a place to add some documentation (though see ARROW-15011)

Reporter: Jonathan Keane / @jonkeane
Assignee: Dewey Dunnington / @paleolimbot

Related issues:

PRs and other links:

Note: This issue was originally created as ARROW-15010. Please see the migration documentation for further details.

@asfimport
Copy link
Collaborator Author

Dewey Dunnington / @paleolimbot:
I think the main thing is to break up the definition of translations into a few files because the current file is getting quite long and contains some translation logic and some actual translations. This also helps with managing multiple function translation PRs (and the testing is separated anyway).

Short reprex of what I was thinking:

nse_funcs <- new.env(parent = emptyenv())
agg_funcs <- new.env(parent = emptyenv())

translation_registry <- function() {
  nse_funcs
}

translation_registry_agg <- function() {
  agg_funcs
}

register_translation <- function(fun_name, fun, registry = translation_registry()) {
  name <- gsub("^.*?::", "", fun_name)
  namespace <- gsub("::.*$", "", fun_name)
  
  attr(fun, "arrow_namespace") <- namespace
  nse_funcs[[name]] <- fun
  invisible(fun)
}

register_translation_agg <- function(fun_name, fun, registry = translation_registry_agg()) {
  register_translation(fun_name, fun, registry = registry)
}

# ...then one of these per package
register_lubridate_translations <- function() {
  register_translation("lubridate::year", function(x) {
    # ...
  })
}

# ...then define them at package load
.onLoad <- function(...) {
  register_lubridate_translations()
}

This has some nice features:

  • You can define related functions, aggregate functions, and (soon!) window functions next to each other
  • No need to worry about collate order (you're the onLoad thing already, caching all the functions into .cache$funcs)
  • You can export register_translation() and make translating to Arrow expressions another package's problem (e.g., geoarrow translations for st_literally_everything())
  • There's a place for namespacing if or when that gets supported

@asfimport
Copy link
Collaborator Author

Jonathan Keane / @jonkeane:
Issue resolved by pull request 11904
#11904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants