paleolimbot commented on code in PR #13160:
URL: https://github.com/apache/arrow/pull/13160#discussion_r914803637
##########
r/tests/testthat/test-dataset-dplyr.R:
##########
@@ -73,7 +73,7 @@ test_that("filter() on timestamp columns", {
ds <- open_dataset(dataset_dir, partitioning = schema(part = uint8()))
expect_equal(
ds %>%
- filter(ts >= lubridate::ymd_hms("2015-05-04 03:12:39")) %>%
+ filter(ts >= lubridate::ymd_hms("2015-05-04 03:12:39", tz = "UTC")) %>%
Review Comment:
Is this change related to this PR?
##########
r/R/arrow-datum.R:
##########
@@ -103,10 +103,10 @@ Ops.ArrowDatum <- function(e1, e2) {
#' @export
Math.ArrowDatum <- function(x, ..., base = exp(1), digits = 0) {
switch(.Generic,
- abs = ,
- sign = ,
- floor = ,
- ceiling = ,
+ abs = eval_array_expression("abs_checked", x),
+ sign = eval_array_expression("sign", x),
+ floor = eval_array_expression("floor", x),
+ ceiling = eval_array_expression("ceil", x),
Review Comment:
This change seems unrelated to this PR. If it's not directly related to the
`pkg::` thing, you should remove it.
##########
r/R/dplyr-funcs.R:
##########
@@ -116,3 +124,11 @@ create_binding_cache <- function() {
nse_funcs <- new.env(parent = emptyenv())
agg_funcs <- new.env(parent = emptyenv())
.cache <- new.env(parent = emptyenv())
+
+register_bindings_utils <- function() {
+ register_binding("::", function(lhs, rhs) {
+ lhs_name <- as.character(substitute(lhs))
+ rhs_name <- as.character(substitute(rhs))
+ nse_funcs[[paste0(lhs_name, "::", rhs_name)]]
Review Comment:
Here you should check if the function is actually in `nse_funcs`...if it
isn't, you should return what base R would return which I think is
`asNamespace(lhs_name)[[rhs_name]]`.
(this is what's causing what you thought was an error in `filter()` but I
think also affected `mutate()`)
##########
r/tests/testthat/test-dplyr-filter.R:
##########
@@ -198,7 +198,7 @@ test_that("Negative scalar values", {
test_that("filter() with between()", {
compare_dplyr_binding(
.input %>%
- filter(between(dbl, 1, 2)) %>%
+ filter(dplyr::between(dbl, 1, 2)) %>%
Review Comment:
I wouldn't change this..."normal" usage is the test before the change...if
you want to test namespacing you should do it in separate tests. Here and below
you are testing two things at once: the behaviour of between and the behaviour
of the namespacing. The namespacing is a separate concern and should be tested
separately.
##########
r/tests/testthat/test-dplyr-funcs.R:
##########
@@ -19,21 +19,24 @@ test_that("register_binding() works", {
fake_registry <- new.env(parent = emptyenv())
fun1 <- function() NULL
- expect_null(register_binding("some_fun", fun1, fake_registry))
+ expect_null(register_binding("some.pkg::some_fun", fun1, fake_registry))
expect_identical(fake_registry$some_fun, fun1)
+ expect_identical(fake_registry$`some.pkg::some_fun`, fun1)
- expect_identical(register_binding("some_fun", NULL, fake_registry), fun1)
+ expect_identical(register_binding("some.pkg::some_fun", NULL,
fake_registry), fun1)
expect_false("some_fun" %in% names(fake_registry))
- expect_silent(expect_null(register_binding("some_fun", NULL, fake_registry)))
+ expect_silent(expect_null(register_binding("some.pkg::some_fun", NULL,
fake_registry)))
- expect_null(register_binding("some_pkg::some_fun", fun1, fake_registry))
+ expect_null(register_binding("somePkg::some_fun", fun1, fake_registry))
expect_identical(fake_registry$some_fun, fun1)
})
test_that("register_binding_agg() works", {
fake_registry <- new.env(parent = emptyenv())
fun1 <- function() NULL
- expect_null(register_binding_agg("some_fun", fun1, fake_registry))
+ expect_null(register_binding_agg("somePkg::some_fun", fun1, fake_registry))
+ names(fake_registry)
Review Comment:
```suggestion
expect_null(register_binding_agg("somePkg::some_fun", fun1, fake_registry))
```
##########
r/R/dplyr-funcs.R:
##########
@@ -58,13 +58,20 @@ NULL
#' @keywords internal
#'
register_binding <- function(fun_name, fun, registry = nse_funcs) {
- name <- gsub("^.*?::", "", fun_name)
- namespace <- gsub("::.*$", "", fun_name)
+ if (fun_name == "::") {
Review Comment:
Maybe change `name` to `qualified_name` here to improve the readability?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
