Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-17689: [R] Implement dplyr::across() inside group_by() #14122

Merged
merged 10 commits into from
Sep 16, 2022
Merged

ARROW-17689: [R] Implement dplyr::across() inside group_by() #14122

merged 10 commits into from
Sep 16, 2022

Conversation

eitsupi
Copy link
Contributor

@eitsupi eitsupi commented Sep 14, 2022 •
edited
Loading

Because the handling of the case .add = TRUE and the add argument have been changed, test cases for these are also added.

@eitsupi
Implement dplyr::across() inside group_by()
557f106 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
add tests
e64da00 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi eitsupi marked this pull request as draft September 14, 2022 14:11
@eitsupi
add tests for the .add option
52617ad 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
fix for empty case
7c87944 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi eitsupi marked this pull request as ready for review September 14, 2022 14:23
@github-actions
Copy link

https://issues.apache.org/jira/browse/ARROW-17689

paleolimbot
paleolimbot reviewed Sep 14, 2022
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.

Thanks! Just a few nits. Excited to have this merged!

r/R/dplyr-group-by.R Outdated Show resolved Hide resolved
r/tests/testthat/test-dplyr-group-by.R Show resolved Hide resolved
r/tests/testthat/test-dplyr-group-by.R Outdated Show resolved Hide resolved
r/R/dplyr-group-by.R Outdated Show resolved Hide resolved
@eitsupi
update add handling and use base::union
58cbc1a 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
update tests
977bb9e 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
add more test for add
8a8d7fb 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
Copy link
Contributor Author

eitsupi commented Sep 15, 2022

Thanks for the review, I have changed the add to match dplyr's behavior and added further tests.

@eitsupi
formatting
6a962e2 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
more tests
f119bb4 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
more tests
9128544 
Signed-off-by: SHIMA Tatsuya <[email protected]>
@eitsupi
Copy link
Contributor Author

eitsupi commented Sep 15, 2022

Further tests were added.
In the process of adding tests, I also found some inappropriate behavior, so I created a ticket in JIRA. (I wanted to add these to the tests as well.)
ARROW-17737
ARROW-17738

paleolimbot
paleolimbot approved these changes Sep 15, 2022
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.

Thank you...this looks great! I will wait for the CI to turn green (minus the duckdb failures) and then merge. In addition to your followups, there's a number of other across-related tickets if you're ever looking (ARROW-17690 and ARROW-17387 are two that come to mind 🙂 ).

@paleolimbot paleolimbot merged commit 557acf5 into apache:master Sep 16, 2022
@eitsupi eitsupi deleted the r-group-across branch September 16, 2022 02:24
@ursabot
Copy link

ursabot commented Sep 16, 2022

Benchmark runs are scheduled for baseline = 93626ee and contender = 557acf5. 557acf5 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️25.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.54% ⬆️0.03%] test-mac-arm
[Failed ⬇️1.13% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.11% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 557acf52 ec2-t3-xlarge-us-east-2
[Finished] 557acf52 test-mac-arm
[Failed] 557acf52 ursa-i9-9960x
[Finished] 557acf52 ursa-thinkcentre-m75q
[Finished] 93626eeb ec2-t3-xlarge-us-east-2
[Failed] 93626eeb test-mac-arm
[Failed] 93626eeb ursa-i9-9960x
[Finished] 93626eeb ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot
Copy link

ursabot commented Sep 16, 2022

['Python', 'R'] benchmarks have high level of regressions.
ec2-t3-xlarge-us-east-2

zagto pushed a commit to zagto/arrow that referenced this pull request Oct 7, 2022
@eitsupi @zagto
ARROW-17689: [R] Implement dplyr::across() inside group_by() (apache#…
ad98203 
…14122)

Because the handling of the case `.add = TRUE` and the `add` argument have been changed, test cases for these are also added.

Authored-by: SHIMA Tatsuya <[email protected]>
Signed-off-by: Dewey Dunnington <[email protected]>
fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
@eitsupi @fatemehp
ARROW-17689: [R] Implement dplyr::across() inside group_by() (apache#…
29a9e1f 
…14122)

Because the handling of the case `.add = TRUE` and the `add` argument have been changed, test cases for these are also added.

Authored-by: SHIMA Tatsuya <[email protected]>
Signed-off-by: Dewey Dunnington <[email protected]>
@asfimport asfimport mentioned this pull request Sep 29, 2022
Closed
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
Component: R
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eitsupi @ursabot @paleolimbot