GH-36720: [R] stringr modifier functions cannot be called with namespace prefix by thisisnic · Pull Request #36758 · apache/arrow

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic · 2023-07-18T19:05:05Z

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: [R] stringr modifier functions cannot be called with namespace prefix #36720

github-actions · 2023-07-18T19:05:31Z

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

paleolimbot

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

Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

thisisnic · 2023-07-20T11:37:18Z

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

paleolimbot · 2023-07-20T13:32:43Z

+ function_called <- call_name(pattern[1])
+
+ if (function_called %in% modifier_funcs) {
+ pattern[1] <- call2(function_called)
+ }


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

Nice catch, will chuck this into a follow-up

conbench-apache-arrow · 2023-07-28T18:25:39Z

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:

Commit Run on ursa-i9-9960x at 2023-07-28 15:43:30Z
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-02, scale_factor=1
- engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-10, scale_factor=1
and 3 more (see the report linked below)

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.

…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 <thisisnic@gmail.com> Co-authored-by: Dewey Dunnington <dewey@dunnington.ca> Signed-off-by: Nic Crane <thisisnic@gmail.com>

thisisnic added 2 commits July 18, 2023 19:28

Add failing tests
98f9d53

Add code to substitute out stringr:: in stringr::regex etc
a418272

thisisnic requested a review from paleolimbot as a code owner July 18, 2023 19:05

github-actionsBot added Component: R awaiting committer review Awaiting committer review labels Jul 18, 2023

Refactor to fit on 1 line and reduce duplication
d03a748

thisisnic marked this pull request as draft July 19, 2023 10:24

Add extra checks to catch non-namespaced calls and non-call patterns
6710fc8

thisisnic marked this pull request as ready for review July 19, 2023 14:01

paleolimbot approved these changes Jul 19, 2023
View reviewed changes

Comment threadr/R/dplyr-funcs-string.R Outdated

github-actionsBot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 19, 2023

Update r/R/dplyr-funcs-string.R
7eefce5
Co-authored-by: Dewey Dunnington <dewey@dunnington.ca>

github-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jul 20, 2023

Remove code to simplify function
84e3e87

github-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jul 20, 2023

Remove unused import
9e80fb8

thisisnic merged commit 819b7d5 into apache:mainJul 20, 2023

paleolimbot reviewed Jul 20, 2023
View reviewed changes

github-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jul 20, 2023