GH-33432: [R] Match base/stringr semantics for str_replace() with NA replacement - #51197
GH-33432: [R] Match base/stringr semantics for str_replace() with NA replacement#51197Gosling-dude wants to merge 1 commit into
Conversation
|
|
There was a problem hiding this comment.
🟢 Approval recommended
The change is localized to the R binding helper, matches the stated semantics, and is covered by a new regression test exercising the key cases.
Pull request overview
Aligns Arrow’s R dplyr bindings for sub()/gsub() and stringr::str_replace()/str_replace_all() with base R/stringr semantics when the replacement is NA_character_, so matched elements become NA instead of splicing the literal "NA" into the string.
Changes:
- Special-cases
replacement = NAin the shared string replacement binding helper by rewriting the operation toif_else(<pattern matches>, NA, x)usingmatch_substring[_regex]. - Adds a regression test covering regex, fixed, and ignore-case patterns, plus empty-string and
NAinputs.
File summaries
| File | Description |
|---|---|
| r/R/dplyr-funcs-string.R | Rewrites NA-replacement string replacement bindings to return NA on match via match_substring[_regex] + if_else. |
| r/tests/testthat/test-dplyr-funcs-string.R | Adds test coverage verifying Arrow matches base/stringr behavior for NA replacements across pattern modes. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…th NA replacement base::sub()/gsub() and stringr::str_replace()/str_replace_all() set the whole string to NA when the replacement is NA and the pattern matches. The Acero replace_substring[_regex] kernels instead splice a literal "NA" into the string. Special-case an NA replacement in the binding and rewrite it as if_else(<pattern matches>, NA, x). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
6986eb9 to
d9f322e
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change is localized, aligns with documented base/stringr semantics, and includes targeted regression tests covering the intended cases.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Rationale for this change
base::sub()/gsub()andstringr::str_replace()/str_replace_all()set the wholeelement to
NAwhen the replacement isNAand the pattern matches. The Aceroreplace_substring[_regex]kernels don't support that and splice a literal"NA"into the string, e.g.
str_replace("one", "o", NA_character_)returns"NAne"instead of
NA.What changes are included in this PR?
Special-case an
NAreplacementin the sharedsub/gsub/str_replace/str_replace_allbinding helper and rewrite it as
if_else(<pattern matches>, NA, x)usingmatch_substring[_regex]. Covers regex, fixed, and ignore-case patterns.Are these changes tested?
Yes — new test in
test-dplyr-funcs-string.Rcomparing against base/stringr forregex/fixed/ignore-case, empty strings, and
NAinput.Are there any user-facing changes?
Bug fix only;
str_replace(x, p, NA)now returnsNAon match instead of acorrupted string.
🤖 Generated with Claude Code