ARROW-14575: [R] Allow functions with pkg:: prefixes

[dragosmg]

This PR will allow the use of namespacing with bindings:

library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)
library(lubridate, warn.conflicts = FALSE)

test_df <- tibble(
  date = as.Date(c("2022-03-22", "2021-07-30", NA))
)

test_df %>%
  mutate(ddate = lubridate::as_datetime(date)) %>%
  collect()
#> # A tibble: 3 × 2
#>   date       ddate              
#>   <date>     <dttm>             
#> 1 2022-03-22 2022-03-22 00:00:00
#> 2 2021-07-30 2021-07-30 00:00:00
#> 3 NA         NA

test_df %>%
  arrow_table() %>% 
  mutate(ddate = lubridate::as_datetime(date)) %>%
  collect()
#> # A tibble: 3 × 2
#>   date       ddate              
#>   <date>     <dttm>             
#> 1 2022-03-22 2022-03-22 00:00:00
#> 2 2021-07-30 2021-07-30 00:00:00
#> 3 NA         NA

^{Created on 2022-05-14 by the reprex package (v2.0.1)}

The approach (option 1 from the design doc):

• add functionality to allow binding registration with the pkg::fun() name;

  • Modify register_binding() to register 2 identical copies for each pkg::fun binding, namely fun and pkg::fun.
  • Add a binding for the :: operator, which helps with retrieving bindings from the function registry.
  • Add generic unit tests for the pkg::fun functionality.
  • Warn for a duplicated binding registration.

• register nse_funcs requiring indirect mapping

  • register each binding with and without the pkg:: prefix.
  • add / update unit tests for the nse_funcs bindings to include at least one pkg::fun() call for each binding
  unit tests for conditional bindings

  • "dplyr::coalesce"
  • "dplyr::if_else"
  • "base::ifelse"
  • "dplyr::case_when"
  unit tests for date/time bindings

  • "base::strptime"
  • "base::strftime"
  • "lubridate::format_ISO8601"
  • "lubridate::is.Date"
  • "lubridate::is.instant"
  • "lubridate::is.timepoint"
  • "lubridate::is.POSIXct"
  • "lubridate::date"
  • "lubridate::second"
  • "lubridate::wday"
  • "lubridate::week"
  • "lubridate::month"
  • "lubridate::am"
  • "lubridate::pm"
  • "lubridate::tz"
  • "lubridate::semester"
  • "lubridate::make_datetime"
  • "lubridate::make_date"
  • "base::ISOdatetime"
  • "base::ISOdate"
  • "base::as.Date"
  • "lubridate::as_date"
  • "lubridate::as_datetime"
  • "lubridate::decimal_date"
  • "lubridate::date_decimal"
  • "base::difftime"
  • "base::as.difftime"
  • "lubridate::make_difftime"
  • "lubridate::dminutes"
  • "lubridate::dhours"
  • "lubridate::ddays"
  • "lubridate::dweeks"
  • "lubridate::dmonths"
  • "lubridate::dyears"
  • "lubridate::dseconds"
  • "lubridate::dmilliseconds"
  • "lubridate::dmicroseconds"
  • "lubridate::dnanoseconds"
  • "lubridate::dpicoseconds"
  • "lubridate::parse_date_time"
  • "lubridate::ymd"
  • "lubridate::ydm"
  • "lubridate::mdy"
  • "lubridate::myd"
  • "lubridate::dmy"
  • "lubridate::dym"
  • "lubridate::ym"
  • "lubridate::my"
  • "lubridate::yq"
  • "lubridate::fast_strptime"
  unit tests for math bindings

  • "base::log"
  • "base::logb"
  • "base::pmin"
  • "base::pmax"
  • "base::trunc"
  • "base::round"
  • "base::sqrt"
  • "base::exp"
  unit tests for string bindings

  • "base::paste"
  • "base::paste0"
  • "stringr::str_c"
  • "base::grepl"
  • "stringr::str_detect"
  • "stringr::str_like"
  • "stringr::str_count"
  • "base::startsWith"
  • "base::endsWith"
  • "stringr::str_starts"
  • "stringr::str_ends"
  • "base::sub"
  • "base::gsub"
  • "stringr::str_replace"
  • "stringr::str_replace_all"
  • "base::strsplit"
  • "stringr::str_split"
  • "base::nchar"
  • "stringr::str_to_lower"
  • "stringr::str_to_upper"
  • "stringr::str_to_title"
  • "stringr::str_trim"
  • "base::substr"
  • "base::substring"
  • "stringr::str_sub"
  • "stringr::str_pad"
  unit tests for type bindings

  • "base::as.character"
  • "base::as.double"
  • "base::as.integer"
  • "bit64::as.integer64"
  • "base::as.logical"
  • "base::as.numeric"
  • "methods::is"
  • "tibble::tibble"
  • "base::data.frame"
  • "base::is.character"
  • "base::is.numeric"
  • "base::is.double"
  • "base::is.integer"
  • "bit64::is.integer64"
  • "base::is.logical"
  • "base::is.factor"
  • "base::is.list"
  • "rlang::is_character"
  • "rlang::is_double"
  • "rlang::is_integer"
  • "rlang::is_list"
  • "rlang::is_logical"
  • "base::is.na"
  • "base::is.nan"
  • "dplyr::between"
  • "base::is.finite"
  • "base::is.infinite"
  • "base::format"

• register nse_funcs requiring direct mapping (unary and binary bindings)

  • register unary bindings
  • register binary bindings
  • add / update unit tests for the nse_funcs bindings to include at least one pkg::fun() call for each binding
  Unary and binary bindings unit tests

  • arithmetic functions

    • "base::abs"
    • "base::ceiling"
    • "base::floor"
    • "base::log10"
    • "base::log1p"
    • "base::log2"
    • "base::sign"

  • trigonometric functions

    • "base::acos"
    • "base::asin"
    • "base::cos"
    • "base::sin"
    • "base::tan"

  • string functions

    • "stringr::str_length"
    • "stringi::stri_reverse"
    • "base::tolower"
    • "base::toupper"

  • date and time functions

    • "lubridate::day"
    • "lubridate::dst"
    • "lubridate::hour"
    • "lubridate::isoweek"
    • "lubridate::epiweek"
    • "lubridate::isoyear"
    • "lubridate::epiyear"
    • "lubridate::minute"
    • "lubridate::quarter"
    • "lubridate::mday"
    • "lubridate::yday"
    • "lubridate::year"
    • "lubridate::leap_year"

  • type conversion functions

    • "base::as.factor"

  • binary functions

    • "base::strrep"
    • "stringr::str_dup"

• aggregating functions

  • register agg_funcs
  • add unit tests for agg_funcs
  unit tests for aggregating bindings

  • "base::sum"
  • "base::any"
  • "base::all"
  • "base::mean"
  • "stats::sd"
  • "stats::var"
  • "stats::quantile"
  • "stats::median"
  • "dplyr::n_distinct"
  • "dplyr::n"
  • "base::min"
  • "base::max"

• namespace qualified bindings work inside the {dplyr} action verbs:

  • filter()
  • mutate()
  • transmute()
  • group_by()
  • summarise()

• document changes in the Writing bindings article.

  • going forward we should be using pkg::fun when defining a binding, which will register 2 copies of the same binding.

Bindings that will not be registered with a pkg:: prefix:

• type casting, such as cast() or dictionary_encode(), and
• operators (e.g. "!", "==", "!=", ">", ">=", "<", "<=", "&", etc.)

[github-actions]

https://issues.apache.org/jira/browse/ARROW-14575

[github-actions]

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

[paleolimbot]

Just a few thoughts on the approach! This will be very exciting to have implemented.

[dragosmg]

It sounds like having a defused version of the :: operator is the way to go (instead of syntax translation). I worked on a proposal for a :: binding yesterday and will push it soon.

Another design choice would be the location for the bindings. Do we want?

• a single bucket (i.e. nse_funcs) to hold all of them?
• individual bucket per namespace: nse_funcs[[package]]
• a mixed approach:
  • a general bucket: nse_funcs[[all_function]] where all bindings live (in their namespace-unqualified form - i.e. as_datetime()
  • package specific buckets: nse_funcs[[package]]

If someone wants to register a pkg::fun binding, it will be recorded in 2 environments, both as nse_funcs[[pkg]][[fun]] and as nse_funcs[[fun]]. If they register fun, it will only be found in use_funcs[[fun]]. We could also add some logic (inspired by Dewey comments on the Jira ticket) to extract the likely namespace from the attached namespaces.

I am in favour of the mixed single-bucket approach , which would make the :: binding something like.

arrow:::register_binding("::", function(lhs, rhs) {
  lhs_name <- as.character(substitute(lhs))
  rhs_name <- as.character(substitute(rhs))
  arrow:::nse_funcs[[lhs_name]][[rhs_name]]
})

[thisisnic]

Given that currently we use a "single-bucket" approach, I'd keep it that way in the short term, unless there's a specific reason to change it? We can update it later if necessary.

[dragosmg]

Given that currently we use a "single-bucket" approach, I'd keep it that way in the short term, unless there's a specific reason to change it? We can update it later if necessary.

Yaya, that's what I'm working on. I went for the mixed bucket approach yesterday evening and earlier today and got really messy really quickly.

[paleolimbot]

I quite liked your suggestion of the single bucket but registering twice (i.e., do nse_funcs[["fun_name"]] <- fun but also nse_funcs[["pkgname::fun_name"]] <- fun if pkgname exists. There's less bookkeeping that way that could possibly get mucked up!

[paleolimbot]

(Since starting to write that note I now see that you both have already hashed that out!)

[dragosmg]

@paleolimbot @thisisnic @jonkeane Would you mind having another look? I can't request a review from @paleolimbot

[paleolimbot]

Looking good...I imagine the next step is to add the function prefixes (including base::) to all the regsiter_binding() calls and add some tests (maybe one per package?) to make sure that when somebody does call lubridate::as_datetime() that it works as we expect.

[dragosmg]

Looking good...I imagine the next step is to add the function prefixes (including base::) to all the regsiter_binding() calls and add some tests (maybe one per package?) to make sure that when somebody does call lubridate::as_datetime() that it works as we expect.

Yep, that's the plan. There are some tests failing - I need to deal with that too.

[dragosmg]

outstanding issues:

• namespacing does seem to work properly for lubridate::date (base::date() is being picked up when not prefixed) - see CI failure https://github.com/apache/arrow/runs/6526882948?check_suite_focus=true. Or maybe it does work (this is the expected behaviour) and we should use the lubridate:: prefix. I'm tired (and I will need to come back to this)

arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 918 to 922 in b7507c3

	# we can't (for now) use namespacing, so we need to make sure lubridate::date()
	# and not base::date() is being used. This is due to the way testthat runs and
	# normal use of arrow would not have to do this explicitly.
	# TODO remove once https://issues.apache.org/jira/browse/ARROW-14575 is done
	date <- lubridate::date

• prefixing doesn't work now works for summarising / aggregating functions too

[dragosmg]

I have no issue with either lubridate::date or date calls in interactive sessions. I think failures for the un-prefixed form are due to the way testthat works, so we shouldn't be concerned with those. Am I wrong?

[paleolimbot]

I think you're safe to make a user prefix lubridate::date() if they do run into this error (and you're also safe to do this in the test, since it's what I'd expect a user to do in the case of an ambiguous function name).

[dragosmg]

I think this is ready for another look, with the following caveats:

• I still need to write a short paragraph / sentence for the Writing bindings article indicating bindings should be registered with "pkg::fun" name.
• using the pkg:: prefix doesn't yet work inside summarise() (i.e. with aggregating functions)
• there is a second approach to achieving the same goal, which can be seen in ARROW-14575 [R] Allow functions with {{pkg::}} prefixes - second approach #13513

[paleolimbot]

A few next steps for you...getting this to work in summarise() will be a set of changes with a different scope because it does its own inspection of the syntax tree. I think you should open another JIRA for it and implement it in a separate PR. I think the filter() issue you identified is actually a general issue and I think one of the review comments I left will fix it.

[dragosmg]

@nealrichardson I think I addressed all the feedback. I'm hoping I can maybe run the CI over the weekend if the r-hub image gets updated.

[nealrichardson]

Very nice work! CI failures are unrelated, and since this fixes the lint issue that has appeared elsewhere, I'm inclined to merge sooner than later.

[nealrichardson]

@github-actions crossbow submit test-r-minimal-build test-r-versions

[github-actions]

Revision: ac316ee

Submitted crossbow builds: ursacomputing/crossbow @ actions-6a3852ff25

Task	Status
test-r-minimal-build
test-r-versions

[ursabot]

Benchmark runs are scheduled for baseline = 29cc263 and contender = 3e0eea1. 3e0eea1 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.38% ⬆️0.03%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.14% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 3e0eea12 ec2-t3-xlarge-us-east-2
[Failed] 3e0eea12 test-mac-arm
[Failed] 3e0eea12 ursa-i9-9960x
[Finished] 3e0eea12 ursa-thinkcentre-m75q
[Finished] 29cc2630 ec2-t3-xlarge-us-east-2
[Failed] 29cc2630 test-mac-arm
[Failed] 29cc2630 ursa-i9-9960x
[Finished] 29cc2630 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java