nealrichardson commented on code in PR #43634: URL: https://github.com/apache/arrow/pull/43634#discussion_r1718749388
########## dev/tasks/r/github.linux.extra.packages.yml: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +{% import 'macros.jinja' as macros with context %} + +{{ macros.github_header() }} + +jobs: + extra-packages: + name: "extra package roundtrip tests" + runs-on: ubuntu-24.04 Review Comment: Future proof? (Assuming setup-r-dependencies handles PPPM setup) ```suggestion runs-on: ubuntu-latest ``` ########## r/tests/testthat/test-extra-package-roundtrip.R: ########## @@ -0,0 +1,108 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +skip_on_cran() + +# Any additional package that we test here that is not already in DESCRIPTION should be +# added to dev/tasks/r/github.linux.extra.packages.yml in the r-lib/actions/setup-r-dependencies@v2 +# step so that they are installed + available in that CI job. + +# So that we can force these in CI +load_or_skip <- function(pkg) { + if (identical(tolower(Sys.getenv("ARROW_R_FORCE_EXTRA_PACKAGE_TESTS")), "true")) { Review Comment: I thought a reason you put this in the regular test suite is so that they would run in local dev if you had the packages installed. But I won't have this env var set locally. Should this check `NOT_CRAN` maybe? ########## r/tests/testthat/test-extra-package-roundtrip.R: ########## @@ -0,0 +1,108 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +skip_on_cran() + +# Any additional package that we test here that is not already in DESCRIPTION should be +# added to dev/tasks/r/github.linux.extra.packages.yml in the r-lib/actions/setup-r-dependencies@v2 +# step so that they are installed + available in that CI job. + +# So that we can force these in CI +load_or_skip <- function(pkg) { + if (identical(tolower(Sys.getenv("ARROW_R_FORCE_EXTRA_PACKAGE_TESTS")), "true")) { + # because of this indirection on the package name we also avoid a CHECK note and + # we don't otherwise need to Suggest this + requireNamespace(pkg, quietly = TRUE) + } else { + skip_if(!requireNamespace(pkg, quietly = TRUE)) + } + attachNamespace(pkg) +} + +library(tibble) + +test_that("readr read csvs roundtrip", { + load_or_skip("readr") + + tbl <- example_data[, c("dbl", "lgl", "false", "chr")] + + tf <- tempfile() + on.exit(unlink(tf)) + write.csv(tbl, tf, row.names = FALSE) + + # we should still be able to turn this into a table + new_df <- read_csv(tf, show_col_types = FALSE) + expect_equal(new_df, as_tibble(arrow_table(new_df))) + + # we should still be able to turn this into a table + new_df <- read_csv(tf, show_col_types = FALSE, lazy = TRUE) + expect_equal(new_df, as_tibble(arrow_table(new_df))) + + # and can roundtrip to a parquet file Review Comment: Minor observation: we already test elsewhere (in the R package and in C++) that writing an Arrow Table to Parquet and back preserves Arrow metadata. So you could just write arrow files. Following that reasoning further, there should be more than enough test coverage in C++ that writing and then reading IPC files preserves all information in memory. So would it be sufficient to just `df |> arrow_table() |> as.data.frame()`? ########## dev/tasks/r/github.linux.extra.packages.yml: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +{% import 'macros.jinja' as macros with context %} + +{{ macros.github_header() }} + +jobs: + extra-packages: + name: "extra package roundtrip tests" + runs-on: ubuntu-24.04 + strategy: + fail-fast: false + env: + ARROW_R_DEV: "FALSE" + RSPM: "https://packagemanager.posit.co/cran/__linux__/noble/latest"; Review Comment: Do you need to set this manually? I thought that these days, `r-lib/actions/setup-r-dependencies` sorts this out for you by default. -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
