Skip to content

Comments

GH-36720: [R] stringr modifier functions cannot be called with namespace prefix#36758

Merged
thisisnic merged 7 commits intoapache:mainfrom
thisisnic:GH-36720-stringr_bindings
Jul 20, 2023
Merged

GH-36720: [R] stringr modifier functions cannot be called with namespace prefix#36758
thisisnic merged 7 commits intoapache:mainfrom
thisisnic:GH-36720-stringr_bindings

Conversation

@thisisnic
Copy link
Member

@thisisnic thisisnic commented Jul 18, 2023 •
edited by github-actions bot
Loading

Rationale for this change

Bug in implementation of string modified functions caused them to be swallowed when prefixed with stringr namespace

What changes are included in this PR?

Strips out the stringr:: prefix when expressions contain stringr::fixed, stringr::coll, string::boundary or stringr::regex

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

@thisisnic thisisnic requested a review from paleolimbot as a code owner July 18, 2023 19:05
@github-actions
Copy link

github-actions bot commented Jul 18, 2023

⚠️ GitHub issue #36720 has been automatically assigned in GitHub to PR creator.

@thisisnic thisisnic marked this pull request as draft July 19, 2023 10:24
@thisisnic thisisnic marked this pull request as ready for review July 19, 2023 14:01
paleolimbot
paleolimbot approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one suggestion that might simplify things a tiny bit 🙂

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 19, 2023
@thisisnic @paleolimbot
Update r/R/dplyr-funcs-string.R
7eefce5 
Co-authored-by: Dewey Dunnington <[email protected]>
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jul 20, 2023
@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 20, 2023
@thisisnic
Copy link
Member Author

thisisnic commented Jul 20, 2023

CI failure is due to #36787 and not changes in this PR, so I'll merge

@thisisnic thisisnic merged commit 819b7d5 into apache:main Jul 20, 2023
paleolimbot
paleolimbot reviewed Jul 20, 2023
View reviewed changes
r/R/dplyr-funcs-string.R
Comment on lines +76 to +80
function_called <- call_name(pattern[1])

if (function_called %in% modifier_funcs) {
pattern[1] <- call2(function_called)
}
Copy link
Member

@paleolimbot paleolimbot Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bit can be reduced to pattern[1] <- call2(function_called) (since is_call() took care of the name/namespace matching).

Copy link
Member Author

@thisisnic thisisnic Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, will chuck this into a follow-up

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 20, 2023
@conbench-apache-arrow
Copy link

conbench-apache-arrow bot commented Jul 28, 2023

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 819b7d5.

There were 5 benchmark results indicating a performance regression:

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
@thisisnic @paleolimbot @loicalleyne
apacheGH-36720: [R] stringr modifier functions cannot be called with …
6b295aa 
…namespace prefix (apache#36758)

### Rationale for this change

Bug in implementation of string modified functions caused them to be swallowed when prefixed with stringr namespace

### What changes are included in this PR?

Strips out the `stringr::` prefix when expressions contain `stringr::fixed`, `stringr::coll`, `string::boundary` or `stringr::regex`

### Are these changes tested?

Yes

### Are there any user-facing changes?

Yes
* Closes: apache#36720

Lead-authored-by: Nic Crane <[email protected]>
Co-authored-by: Dewey Dunnington <[email protected]>
Signed-off-by: Nic Crane <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@paleolimbot paleolimbot paleolimbot approved these changes

Assignees

No one assigned

Labels

awaiting changes Awaiting changes Component: R

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[R] stringr modifier functions cannot be called with namespace prefix

2 participants

@thisisnic @paleolimbot