paleolimbot commented on a change in pull request #12154:
URL: https://github.com/apache/arrow/pull/12154#discussion_r838554564
##########
File path: r/src/compute.cpp
##########
@@ -521,6 +521,20 @@ std::shared_ptr<arrow::compute::FunctionOptions>
make_compute_options(
return out;
}
+ if (func_name == "round_temporal" || func_name == "floor_temporal" ||
+ func_name == "ceil_temporal") {
+ using Options = arrow::compute::RoundTemporalOptions;
+ auto out = std::make_shared<Options>(Options::Defaults());
+ SEXP unit = options["unit"];
+ if (!Rf_isNull(unit)) {
Review comment:
A little like the above...it's probably fine to call `Rf_isNull()` here
because it's unlikely that `Rf_isNull()` will call `Rf_error()` (which will
`longjmp` and maybe leak memory here), but it's hard to guess which R API calls
will do this and it changes frequently. You could also
do`cpp11::safe[Rf_isNull](unit)`, although I think `unit == R_NilValue` is a
bit more readable. Apologies if I'm explaining something you already know!
```suggestion
if (unit == R_NilValue) {
```
##########
File path: r/src/compute.cpp
##########
@@ -521,6 +521,20 @@ std::shared_ptr<arrow::compute::FunctionOptions>
make_compute_options(
return out;
}
+ if (func_name == "round_temporal" || func_name == "floor_temporal" ||
+ func_name == "ceil_temporal") {
+ using Options = arrow::compute::RoundTemporalOptions;
+ auto out = std::make_shared<Options>(Options::Defaults());
+ SEXP unit = options["unit"];
Review comment:
Tiny nit, but it's important not to mix cpp11 idioms and `SEXP`/other R
API calls (if you were using an SEXP here, I think you would need `PROTECT()`
and `UNPROTECT()`).
```suggestion
cpp11::sexp unit = options["unit"];
```
--
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]
