nealrichardson commented on code in PR #14271:
URL: https://github.com/apache/arrow/pull/14271#discussion_r991510991
##########
r/tests/testthat/test-RecordBatch.R:
##########
@@ -642,14 +642,19 @@ test_that("Handling string data with embedded nuls", {
batch_with_nul$b <- batch_with_nul$b$cast(utf8())
withr::with_options(list(arrow.skip_nul = TRUE), {
- expect_warning(
- expect_equal(
- as.data.frame(batch_with_nul)$b,
- c("person", "woman", "man", "camera", "tv"),
- ignore_attr = TRUE
- ),
- "Stripping '\\0' (nul) from character vector",
- fixed = TRUE
+ # Because expect_equal() may call identical(x, y) more than once,
Review Comment:
"may"? Is it not deterministic? Why would it do the work multiple times?
Would it help to change the test, like
`expect_true(all(as.data.frame(batch_with_nul)$b == c("person", "woman", "man",
"camera", "tv")))`?
##########
r/tests/testthat/test-altrep.R:
##########
@@ -54,6 +80,54 @@ test_that("altrep vectors from int32 and dbl arrays with no
nulls", {
expect_false(is_arrow_altrep(as.vector(v_dbl$Slice(1))))
})
+test_that("element access methods for int32 ALTREP with no nulls", {
+ withr::local_options(list(arrow.use_altrep = TRUE))
+ original <- 1:1000
Review Comment:
Not that it matters much, but does this array need 1000 elements to test the
behavior?
##########
r/src/array.cpp:
##########
@@ -69,7 +69,10 @@ void arrow::r::validate_slice_length(R_xlen_t length,
int64_t available) {
cpp11::stop("Slice 'length' cannot be negative");
}
if (length > available) {
- cpp11::warning("Slice 'length' greater than available length");
+ // For an unknown reason, cpp11::warning() crashes here; however, this
+ // should throw an exception if Rf_warning() jumps, so we need
+ // cpp11::safe[]().
+ cpp11::safe[Rf_warning]("Slice 'length' greater than available length");
Review Comment:
Oh I see, it's not just this warning, there are other warnings we care about.
##########
r/src/array.cpp:
##########
@@ -69,7 +69,10 @@ void arrow::r::validate_slice_length(R_xlen_t length,
int64_t available) {
cpp11::stop("Slice 'length' cannot be negative");
}
if (length > available) {
- cpp11::warning("Slice 'length' greater than available length");
+ // For an unknown reason, cpp11::warning() crashes here; however, this
+ // should throw an exception if Rf_warning() jumps, so we need
+ // cpp11::safe[]().
+ cpp11::safe[Rf_warning]("Slice 'length' greater than available length");
Review Comment:
If raising a warning could crash, should we just skip the warning?
--
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]
