GH-51655: [R] Advance long-standing deprecations in map_batches() and pull() - #51658
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The public API changes need a current release-note entry.
Review effort: Balanced
Findings: 1
What changed in this PR
Keeps pull() returning R vectors by default and removes the deprecated map_batches() argument.
Changes:
- Removes
.data.framefrommap_batches(). - Finalizes
pull()defaults and documentation. - Updates related tests.
| File | Description |
|---|---|
r/R/dataset-scan.R |
Removes deprecated argument. |
r/R/dplyr-collect.R |
Defaults pull() to vectors. |
r/R/dplyr-funcs-doc.R |
Updates pull() documentation. |
r/R/arrow-package.R |
Updates method summary. |
r/man/map_batches.Rd |
Updates generated reference. |
r/man/acero.Rd |
Updates generated reference. |
r/tests/testthat/helper-arrow.R |
Removes obsolete test option. |
r/tests/testthat/test-dplyr-query.R |
Tests finalized behavior. |
Files not reviewed (2)
- r/man/acero.Rd: Generated file
- r/man/map_batches.Rd: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation, tests, documentation, and release notes consistently reflect the intended API behavior.
Review effort: Balanced
Findings: 1
Files not reviewed (2)
- r/man/acero.Rd: Generated file
- r/man/map_batches.Rd: Generated file
jonkeane
left a comment
jonkeane
left a comment
There was a problem hiding this comment.
Thanks for this. My question at the end of my one comment is actually answered in the news down below, but I'm going to leave it in the comment for posterity (which is recursive posterity, apparently)
| } | ||
|
|
||
| handle_pull_as_vector <- function(out, as_vector) { | ||
| if (is.null(as_vector)) { |
There was a problem hiding this comment.
I did some digging (well tasked a clanker to do the digging...) to see why we did this in the first place. Here it is for posterity (or some future clanker to dig it up). We originally proposed to do this since there wasn't another ergonomic way to get a chunked array, and unlike dbplyr where this was originally inspired by, Arrow has an in-memory representation, so there wasn't a requirement to turn it into an R vector.
It turns out that broke dependencies so we made it opt in at the time, though there has been consistent feedback that other engines like data.table also return just a vector (and conceptually dplyr does this with data.frames too, it strips the outer data.frameness).
I'm good with undeprecating this. Though I am slightly curious (and we should do this in a follow on if we do), do we now have a (more) ergonomic way to get a chunked array? is it pull(..., as_vector=FALSE)? or something else?
github-actions
Bot
added
awaiting merge
Rationale for this change
Old deprecation warnings hadn't been dealt with
What changes are included in this PR?
I've deprecated the
.data.frameargument inmap_batches()but I think we actually may be better off leavingpull()as-is given it's been like that for such a long time.Are these changes tested?
Existing tests
Are there any user-facing changes?
Yes
pull()on Arrow data no longer warns about a future change of default; it will keep returning an R vector by default, withas_vector = FALSEoroptions(arrow.pull_as_vector = FALSE)to get aChunkedArray.The deprecated
.data.frameargument tomap_batches()has been removed.Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Reviewed before submission by: