GH-49237: [R] Deprecate Feather reader and writer - #49276
Conversation
|
|
d3eca24 to
d32a2e3
Compare
d7c26bb to
71a2ff5
Compare
There was a problem hiding this comment.
Pull request overview
This PR deprecates Feather-oriented R APIs in favor of Arrow IPC terminology and updates documentation/tests to use read_ipc_file(), write_ipc_file(), and format = "ipc".
Changes:
- Adds deprecation warnings for
read_feather(),write_feather(), andformat = "feather"dataset usage. - Splits IPC file reader/writer documentation from deprecated Feather docs.
- Updates tests, examples, snapshots, and generated Rd files to use IPC naming.
Reviewed changes
Copilot reviewed 25 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| r/R/feather.R | Refactors Feather/IPc reader-writer APIs and docs. |
| r/R/ipc-stream.R | Updates IPC stream docs and raw-file serialization path. |
| r/R/dataset-write.R | Deprecates format = "feather" for dataset writes. |
| r/R/dataset-format.R | Deprecates Feather format alias and returns ipc for IPC formats. |
| r/R/dataset.R | Updates dataset format documentation and display name. |
| r/R/dataset-factory.R | Updates dataset factory format documentation. |
| r/R/parquet.R | Updates inherited reader parameter docs. |
| r/R/record-batch-reader.R | Updates reader docs to reference IPC file APIs. |
| r/R/record-batch-writer.R | Updates writer docs to reference IPC file APIs. |
| r/R/extension.R | Updates extension examples to use IPC file APIs. |
| r/tests/testthat/test-feather.R | Updates Feather tests for deprecation and IPC replacements. |
| r/tests/testthat/test-dataset.R | Updates dataset IPC tests and adds Feather deprecation coverage. |
| r/tests/testthat/test-dataset-write.R | Updates dataset write tests and adds Feather deprecation coverage. |
| r/tests/testthat/test-read-write.R | Replaces Feather roundtrip usage with IPC file APIs. |
| r/tests/testthat/test-read-record-batch.R | Replaces Feather reader usage with IPC file reader. |
| r/tests/testthat/test-metadata.R | Replaces Feather metadata roundtrips with IPC roundtrips. |
| r/tests/testthat/test-s3.R | Updates S3 IPC read/write test naming and APIs. |
| r/tests/testthat/test-utf.R | Replaces Feather serialization test with IPC file APIs. |
| r/tests/testthat/test-extension.R | Updates extension roundtrip to use IPC file APIs. |
| r/tests/testthat/test-dplyr-mutate.R | Updates dataset format usage to IPC. |
| r/tests/testthat/test-buffer.R | Replaces Feather writer with IPC file writer. |
| r/tests/testthat/test-backwards-compatibility.R | Reads Feather golden files via IPC file reader. |
| r/tests/testthat/test-Array.R | Updates timezone roundtrip to use IPC file APIs. |
| r/tests/testthat/helper-filesystems.R | Updates filesystem IPC read/write tests. |
| r/tests/testthat/_snaps/dataset-write.md | Updates snapshots for IPC format calls. |
| r/man/write_ipc_file.Rd | Adds generated docs for write_ipc_file(). |
| r/man/read_ipc_file.Rd | Adds generated docs for read_ipc_file(). |
| r/man/write_feather.Rd | Updates generated docs for deprecated writer. |
| r/man/read_feather.Rd | Updates generated docs for deprecated reader. |
| r/man/write_ipc_stream.Rd | Updates IPC stream writer docs. |
| r/man/read_ipc_stream.Rd | Updates IPC stream reader docs. |
| r/man/write_to_raw.Rd | Updates raw serialization docs. |
| r/man/write_dataset.Rd | Updates dataset write format docs. |
| r/man/open_dataset.Rd | Updates dataset opening format docs. |
| r/man/FileFormat.Rd | Updates file format docs. |
| r/man/dataset_factory.Rd | Updates dataset factory docs. |
| r/man/RecordBatchReader.Rd | Updates reader reference docs. |
| r/man/RecordBatchWriter.Rd | Updates writer reference docs. |
| r/man/vctrs_extension_array.Rd | Updates extension example docs. |
Files not reviewed (14)
- r/man/FileFormat.Rd: Language not supported
- r/man/RecordBatchReader.Rd: Language not supported
- r/man/RecordBatchWriter.Rd: Language not supported
- r/man/dataset_factory.Rd: Language not supported
- r/man/open_dataset.Rd: Language not supported
- r/man/read_feather.Rd: Language not supported
- r/man/read_ipc_file.Rd: Language not supported
- r/man/read_ipc_stream.Rd: Language not supported
- r/man/vctrs_extension_array.Rd: Language not supported
- r/man/write_dataset.Rd: Language not supported
- r/man/write_feather.Rd: Language not supported
- r/man/write_ipc_file.Rd: Language not supported
- r/man/write_ipc_stream.Rd: Language not supported
- r/man/write_to_raw.Rd: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jonkeane
left a comment
There was a problem hiding this comment.
A few comments, but this looks good and if CI is happy I think we are ok to merge. I do think we should avoid changing those signatures now and actually have a deprecation cycle in case someone is using them.
0e1a899 to
f27f400
Compare
### Rationale for this change See #49231. Deprecate the Feather reader/writer and point users to the Arrow IPC file API. ### What changes are included in this PR? `ipc::feather::Reader::Open`, `ipc::feather::WriteTable` and the `WriteProperties` struct marked with `ARROW_DEPRECATED`, pointing users to `ipc::RecordBatchFileReader` and `ipc::MakeFileWriter`. Deprecation warnings supressed at the internal call sites: `feather.cc`, `feather_test.cc`, the R binding `r/src/feather.cpp` (R deprecation is in #49276), and the GLib binding (GLib deprecation is in #49673). ### Are these changes tested? `-Werror` build of arrow-feather-test passes locally. ### Are there any user-facing changes? No functional change, compile-time deprecation warning for `feather::Reader::Open`, `feather::WriteTable` or feather::WriteProperties. * GitHub Issue: #49231 Authored-by: Tadeja Kadunc <tadeja.kadunc@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 39 changed files in this pull request and generated 2 comments.
Files not reviewed (14)
- r/man/FileFormat.Rd: Generated file
- r/man/RecordBatchReader.Rd: Generated file
- r/man/RecordBatchWriter.Rd: Generated file
- r/man/dataset_factory.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
- r/man/read_feather.Rd: Generated file
- r/man/read_ipc_file.Rd: Generated file
- r/man/read_ipc_stream.Rd: Generated file
- r/man/vctrs_extension_array.Rd: Generated file
- r/man/write_dataset.Rd: Generated file
- r/man/write_feather.Rd: Generated file
- r/man/write_ipc_file.Rd: Generated file
- r/man/write_ipc_stream.Rd: Generated file
- r/man/write_to_raw.Rd: Generated file
2a8bf3d to
62c0745
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 39 changed files in this pull request and generated 2 comments.
Files not reviewed (14)
- r/man/FileFormat.Rd: Generated file
- r/man/RecordBatchReader.Rd: Generated file
- r/man/RecordBatchWriter.Rd: Generated file
- r/man/dataset_factory.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
- r/man/read_feather.Rd: Generated file
- r/man/read_ipc_file.Rd: Generated file
- r/man/read_ipc_stream.Rd: Generated file
- r/man/vctrs_extension_array.Rd: Generated file
- r/man/write_dataset.Rd: Generated file
- r/man/write_feather.Rd: Generated file
- r/man/write_ipc_file.Rd: Generated file
- r/man/write_ipc_stream.Rd: Generated file
- r/man/write_to_raw.Rd: Generated file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 39 changed files in this pull request and generated 1 comment.
Files not reviewed (14)
- r/man/FileFormat.Rd: Generated file
- r/man/RecordBatchReader.Rd: Generated file
- r/man/RecordBatchWriter.Rd: Generated file
- r/man/dataset_factory.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
- r/man/read_feather.Rd: Generated file
- r/man/read_ipc_file.Rd: Generated file
- r/man/read_ipc_stream.Rd: Generated file
- r/man/vctrs_extension_array.Rd: Generated file
- r/man/write_dataset.Rd: Generated file
- r/man/write_feather.Rd: Generated file
- r/man/write_ipc_file.Rd: Generated file
- r/man/write_ipc_stream.Rd: Generated file
- r/man/write_to_raw.Rd: Generated file
| expect_snapshot( | ||
| write_dataset(df, dst_dir, format = "feather", nonsensical_arg = "blah-blah"), | ||
| write_dataset(df, dst_dir, format = "ipc", nonsensical_arg = "blah-blah"), | ||
| error = TRUE | ||
| ) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 39 changed files in this pull request and generated no new comments.
Files not reviewed (14)
- r/man/FileFormat.Rd: Generated file
- r/man/RecordBatchReader.Rd: Generated file
- r/man/RecordBatchWriter.Rd: Generated file
- r/man/dataset_factory.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
- r/man/read_feather.Rd: Generated file
- r/man/read_ipc_file.Rd: Generated file
- r/man/read_ipc_stream.Rd: Generated file
- r/man/vctrs_extension_array.Rd: Generated file
- r/man/write_dataset.Rd: Generated file
- r/man/write_feather.Rd: Generated file
- r/man/write_ipc_file.Rd: Generated file
- r/man/write_ipc_stream.Rd: Generated file
- r/man/write_to_raw.Rd: Generated file
Comments suppressed due to low confidence (5)
r/R/ipc-stream.R:120
- Same issue as
write_ipc_stream(): calling.Deprecated()without an explicitold=will likely warn thatread_ipc_stream()is deprecated, rather than warning about deprecated extra...arguments.
if (length(list(...)) > 0) {
.Deprecated(
msg = paste(
"Extra arguments passed through `...` in `read_ipc_stream()`",
"are deprecated and ignored.",
"They will be removed in a future version."
)
)
r/R/feather.R:32
write_feather()still supportsversion = 1, but the inherited parameter docs forchunk_size,compression, andcompression_levelno longer mention the V1 restrictions enforced bycheck_feather_v1_options(). This makes the docs inconsistent with actual behavior for users still writing legacy V1 files.
#' @inheritParams write_ipc_file
#' @param version integer Feather file version, Version 1 or Version 2. Version 2 is the default.
#'
r/R/ipc-stream.R:47
.Deprecated()is called without specifyingold=/new=, which means the warning will likely state thatwrite_ipc_stream()itself is deprecated (the defaultold), even though only passing extra...args is being deprecated. This is misleading for users and makes it harder to see what is actually deprecated.
This issue also appears on line 113 of the same file.
if (length(list(...)) > 0) {
.Deprecated(
msg = paste(
"Extra arguments passed through `...` in `write_ipc_stream()`",
"are deprecated and ignored.",
"They will be removed in a future version."
)
)
r/R/dataset-write.R:150
- Using
.Deprecated()here withoutold=/new=will likely emit a warning thatwrite_dataset()is deprecated (becauseolddefaults to the current call), even though onlyformat = "feather"is deprecated. Consider specifyingold/newso the warning points at the deprecated argument value instead of the function.
if (format == "feather") {
.Deprecated(
msg = '`format = "feather"` is deprecated; use `format = "ipc"` instead.'
)
}
r/R/dataset-format.R:93
- As written,
.Deprecated()will likely report thatFileFormat$create()is deprecated (defaultold), rather than warning specifically about the deprecatedformat = "feather"value. Setold=/new=so the warning is scoped to the argument value being deprecated.
if (format == "feather") {
.Deprecated(
msg = '`format = "feather"` is deprecated; use `format = "ipc"` instead.'
)
}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
82901e5 to
ee4b0c3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 44 changed files in this pull request and generated no new comments.
Files not reviewed (14)
- r/man/FileFormat.Rd: Generated file
- r/man/RecordBatchReader.Rd: Generated file
- r/man/RecordBatchWriter.Rd: Generated file
- r/man/dataset_factory.Rd: Generated file
- r/man/open_dataset.Rd: Generated file
- r/man/read_feather.Rd: Generated file
- r/man/read_ipc_file.Rd: Generated file
- r/man/read_ipc_stream.Rd: Generated file
- r/man/vctrs_extension_array.Rd: Generated file
- r/man/write_dataset.Rd: Generated file
- r/man/write_feather.Rd: Generated file
- r/man/write_ipc_file.Rd: Generated file
- r/man/write_ipc_stream.Rd: Generated file
- r/man/write_to_raw.Rd: Generated file
Comments suppressed due to low confidence (3)
r/R/ipc-stream.R:39
length(list(...))forces evaluation of the...arguments. Since these are explicitly ignored, this changes behavior (and can introduce side effects / errors) for callers that pass expensive or erroring expressions. You can detect whether...was supplied without evaluating it.
if (length(list(...)) > 0) {
r/R/ipc-stream.R:113
- Same issue as
write_ipc_stream():length(list(...))evaluates ignored...arguments. Use a non-evaluating check so callers passing extra args don't trigger side effects just from the deprecation warning.
if (length(list(...)) > 0) {
r/R/feather.R:32
- The docs now inherit
chunk_size/compression/compression_levelfromwrite_ipc_file(), butwrite_feather(version = 1)still errors if these are set to non-default values. Adding a brief note to theversionparameter docs would prevent confusion for users still writing Feather V1 during the deprecation period.
#' @inheritParams write_ipc_file
#' @param version integer Feather file version, Version 1 or Version 2. Version 2 is the default.
#'
|
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit deed9c0. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 111 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
Arrow C++ is deprecating the Feather reader/writer (#49231) so we should update the R functions too
What changes are included in this PR?
write_featherto mention in the docs that it's deprecated.write_feather()to wrap new functionwrite_ipc_impl()so we can show deprecation warnings on calls towrite_feather()only (previouslywrite_ipc_file()calledwrite_feather()and so I didn't wanna add the deprecation warning without this refactor otherwise a call towrite_ipc_file()would trigger it erroneously)write_feather()we can remove this extra functionread_ipc_file()andread_feather()into separate functions so we can document them differently. Updated the examples to show usingread_ipc_file()format = "feather"inopen_dataset(),write_dataset(), andFileFormat$create()- now emits deprecation warning recommendingformat = "ipc"insteadas.character.FileFormatto return"ipc"instead of"feather""ipc"instead of"feather"Some 🤖 use here, but I decided on what to do.
Are these changes tested?
Yep
Are there any user-facing changes?
Yep - deprecation warning