ARROW-17460: [R] Don't warn if the new UDF I'm registering is the same as the existing one#14436
Conversation
|
|
Do you think that should be reported as a bug? |
r/R/compute.R
Outdated
| # extra step to avoid saving this execution environment in the binding, | ||
| # which eliminates a warning when the same binding is registered twice | ||
| binding_fun <- function(...) build_expr(name, ...) | ||
| body(binding_fun)[[2]] <- name |
There was a problem hiding this comment.
I pushed a less cryptic version of this...it's inlining the value of name into the function body:
binding_fun <- function(...) build_expr(name, ...)
str(as.list(body(binding_fun)))
#> List of 3
#> $ : symbol build_expr
#> $ : symbol name
#> $ : symbol ...
I opened an issue about it...it might have been intentional since protection isn't necessarily cheap compared to the function call itself. |
|
@github-actions crossbow submit homebrew-r-autobrew |
|
Revision: 0facb9b Submitted crossbow builds: ursacomputing/crossbow @ actions-6b8a20d344
|
|
It looks like it doesn't fix that specific test, although I still think we should merge this sooner than later just in case it was contributing to other failures! |
| arrow::Status ReadNext(std::shared_ptr<arrow::RecordBatch>* batch_out) { | ||
| auto batch = SafeCallIntoR<std::shared_ptr<arrow::RecordBatch>>([&]() { | ||
| cpp11::sexp result_sexp = fun_(); | ||
| cpp11::sexp result_sexp = cpp11::function(fun_)(); |
There was a problem hiding this comment.
Does it potentially defer any type check that would have occurred in the constructor before?
There was a problem hiding this comment.
That's a good point...I looked that up, and its current form, cpp11::function doesn't do any type checking ( https://github.com/r-lib/cpp11/blob/main/inst/include/cpp11/function.hpp#L17 )
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
+1. This looks reasonable on the face of it.
|
@nealrichardson @thisisnic Could you comment on the R parts? |
…e as the existing one (#14436) This PR makes it so that you can do the following without a warning: ``` r library(arrow, warn.conflicts = FALSE) register_scalar_function( "times_32", function(context, x) x * 32L, in_type = list(int32(), int64(), float64()), out_type = function(in_types) in_types[[1]], auto_convert = TRUE ) register_scalar_function( "times_32", function(context, x) x * 32L, in_type = list(int32(), int64(), float64()), out_type = function(in_types) in_types[[1]], auto_convert = TRUE ) ``` In fixing that, I also ran across an important discovery, which is that `cpp11::function` does *not* protect the underlying `SEXP` from garbage collection!!!! It the two functions we used this for were being protected by proxy because the execution environment of `register_scalar_function()` was being saved when the binding was registered. Authored-by: Dewey Dunnington <[email protected]> Signed-off-by: Dewey Dunnington <[email protected]>
3 participants
This PR makes it so that you can do the following without a warning:
In fixing that, I also ran across an important discovery, which is that
cpp11::functiondoes not protect the underlyingSEXPfrom garbage collection!!!! It the two functions we used this for were being protected by proxy because the execution environment ofregister_scalar_function()was being saved when the binding was registered.