From f5651351a13f44c79a506541a8551c0e8dac0bf7 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Sat, 11 Apr 2026 20:28:09 +0100 Subject: [PATCH 1/4] Add proper checking of tzone type --- r/src/type_infer.cpp | 4 ++++ r/tests/testthat/test-type.R | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/r/src/type_infer.cpp b/r/src/type_infer.cpp index a8334387c525..76a1499305ea 100644 --- a/r/src/type_infer.cpp +++ b/r/src/type_infer.cpp @@ -72,6 +72,8 @@ std::shared_ptr InferArrowTypeFromVector(SEXP x) { if (Rf_isNull(tzone_sexp)) { auto systzone_sexp = cpp11::package("base")["Sys.timezone"]; return timestamp(TimeUnit::MICRO, CHAR(STRING_ELT(systzone_sexp(), 0))); + } else if (TYPEOF(tzone_sexp) != STRSXP) { + cpp11::stop("`tzone` attribute of a `POSIXct` vector must be a character vector"); } else { return timestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); } @@ -89,6 +91,8 @@ std::shared_ptr InferArrowTypeFromVector(SEXP x) { if (Rf_isNull(tzone_sexp)) { auto systzone_sexp = cpp11::package("base")["Sys.timezone"]; return timestamp(TimeUnit::MICRO, CHAR(STRING_ELT(systzone_sexp(), 0))); + } else if (TYPEOF(tzone_sexp) != STRSXP) { + cpp11::stop("`tzone` attribute of a `POSIXct` vector must be a character vector"); } else { return timestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0))); } diff --git a/r/tests/testthat/test-type.R b/r/tests/testthat/test-type.R index 96968830f3d0..bebc27178aff 100644 --- a/r/tests/testthat/test-type.R +++ b/r/tests/testthat/test-type.R @@ -53,6 +53,26 @@ test_that("infer_type() infers from R type", { ) }) +test_that("infer_type() errors clearly for POSIXct with invalid tzone", { + x <- as.POSIXct("2019-02-14 13:55:05", tz = "UTC") + attr(x, "tzone") <- 123 + + expect_error( + infer_type(x), + "`tzone` attribute of a `POSIXct` vector must be a character vector" + ) +}) + +test_that("infer_type() errors clearly for zero-length POSIXct with invalid tzone", { + x <- as.POSIXct(x = NULL) + attr(x, "tzone") <- 123 + + expect_error( + infer_type(x), + "`tzone` attribute of a `POSIXct` vector must be a character vector" + ) +}) + test_that("infer_type() default method errors for unknown classes", { vec <- structure(list(), class = "class_not_supported") From b9d695d91a6086d034eb58421d877bf225aaa440 Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Tue, 21 Apr 2026 07:29:10 -0400 Subject: [PATCH 2/4] Combine tests --- r/tests/testthat/test-type.R | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/r/tests/testthat/test-type.R b/r/tests/testthat/test-type.R index bebc27178aff..74c868faac2a 100644 --- a/r/tests/testthat/test-type.R +++ b/r/tests/testthat/test-type.R @@ -61,9 +61,8 @@ test_that("infer_type() errors clearly for POSIXct with invalid tzone", { infer_type(x), "`tzone` attribute of a `POSIXct` vector must be a character vector" ) -}) -test_that("infer_type() errors clearly for zero-length POSIXct with invalid tzone", { + # Also check zero-length POSIXct x <- as.POSIXct(x = NULL) attr(x, "tzone") <- 123 From 3efa79395ccbdc3b8a682b4203a63d794bf8473b Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Tue, 21 Apr 2026 08:16:04 -0400 Subject: [PATCH 3/4] More french into separate block to see if error disappears --- r/tests/testthat/test-dplyr-filter.R | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/r/tests/testthat/test-dplyr-filter.R b/r/tests/testthat/test-dplyr-filter.R index 3912e518ed08..dd7bda6d884b 100644 --- a/r/tests/testthat/test-dplyr-filter.R +++ b/r/tests/testthat/test-dplyr-filter.R @@ -287,11 +287,21 @@ test_that("filter environment scope", { }) test_that("Filtering on a column that doesn't exist errors correctly", { + # expect_warning(., NA) because the usual behavior when it hits a filter + # that it can't evaluate is to raise a warning, collect() to R, and retry + # the filter. But we want this to error the first time because it's + # a user error, not solvable by retrying in R + expect_warning( + expect_error( + tbl |> record_batch() |> filter(not_a_col == 42) |> collect(), + class = "validation_error" + ), + NA + ) +}) + +test_that("Filtering on a non-existent column errors in the correct language", { with_language("fr", { - # expect_warning(., NA) because the usual behavior when it hits a filter - # that it can't evaluate is to raise a warning, collect() to R, and retry - # the filter. But we want this to error the first time because it's - # a user error, not solvable by retrying in R expect_warning( expect_error( tbl |> record_batch() |> filter(not_a_col == 42) |> collect(), @@ -300,15 +310,6 @@ test_that("Filtering on a column that doesn't exist errors correctly", { NA ) }) - with_language("en", { - expect_warning( - expect_error( - tbl |> record_batch() |> filter(not_a_col == 42) |> collect(), - "object 'not_a_col' not found" - ), - NA - ) - }) }) test_that("Filtering with unsupported functions", { From e0389783e0a42c4f7857f57c16223274ae76336b Mon Sep 17 00:00:00 2001 From: Nic Crane Date: Tue, 21 Apr 2026 08:21:09 -0400 Subject: [PATCH 4/4] Revert "More french into separate block to see if error disappears" This reverts commit 3efa79395ccbdc3b8a682b4203a63d794bf8473b. --- r/tests/testthat/test-dplyr-filter.R | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/r/tests/testthat/test-dplyr-filter.R b/r/tests/testthat/test-dplyr-filter.R index dd7bda6d884b..3912e518ed08 100644 --- a/r/tests/testthat/test-dplyr-filter.R +++ b/r/tests/testthat/test-dplyr-filter.R @@ -287,21 +287,11 @@ test_that("filter environment scope", { }) test_that("Filtering on a column that doesn't exist errors correctly", { - # expect_warning(., NA) because the usual behavior when it hits a filter - # that it can't evaluate is to raise a warning, collect() to R, and retry - # the filter. But we want this to error the first time because it's - # a user error, not solvable by retrying in R - expect_warning( - expect_error( - tbl |> record_batch() |> filter(not_a_col == 42) |> collect(), - class = "validation_error" - ), - NA - ) -}) - -test_that("Filtering on a non-existent column errors in the correct language", { with_language("fr", { + # expect_warning(., NA) because the usual behavior when it hits a filter + # that it can't evaluate is to raise a warning, collect() to R, and retry + # the filter. But we want this to error the first time because it's + # a user error, not solvable by retrying in R expect_warning( expect_error( tbl |> record_batch() |> filter(not_a_col == 42) |> collect(), @@ -310,6 +300,15 @@ test_that("Filtering on a non-existent column errors in the correct language", { NA ) }) + with_language("en", { + expect_warning( + expect_error( + tbl |> record_batch() |> filter(not_a_col == 42) |> collect(), + "object 'not_a_col' not found" + ), + NA + ) + }) }) test_that("Filtering with unsupported functions", {