• Closes: [R] arrow returns wrong variable content when multiple group_by/summarise statements are used #14872

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

library(arrow, warn.conflicts = FALSE)
#> Some features are not enabled in this build of Arrow. Run `arrow_info()` for more information.
library(dplyr, warn.conflicts = FALSE)

mtcars |>
  arrow_table() |>
  select(mpg, cyl) |> 
  group_by(mpg, cyl) |>
  group_by(cyl, value = "foo") |>
  collect()

Wouldn't it be better to add this example to the test?

I did add a test that covers that case (and made sure it failed on current master); however, the test I added matches the style and content the tests around it (e.g., they all use example_data and compare_dplyr_binding()). Is there something that the example is testing that wasn't covered by the test that I added that I missed?

Is there something that the example is testing that wasn't covered by the test that I added that I missed?

I thought an example of defining a new column in group_by would that case.
It seems quite surprising that both value and "foo" columns are generated in the following example.

library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)

mtcars |>
  arrow_table() |>
  select(mpg, cyl) |> 
  group_by(mpg, cyl) |>
  group_by(cyl, value = "foo") |>
  collect()

I thought an example of defining a new column in group_by would that case. It seems quite surprising that both value and "foo" columns are generated in the following example.

library(arrow, warn.conflicts = FALSE)
library(dplyr, warn.conflicts = FALSE)

mtcars |>
  arrow_table() |>
  select(mpg, cyl) |> 
  group_by(mpg, cyl) |>
  group_by(cyl, value = "foo") |>
  collect()

Good catch there, though that looks like a bug which exists separately from this PR as I was able to reproduce this on the tip of the master branch. @paleolimbot - wanna take a look at this here, or shall we open a separate issue and come back to this?

It's definitely worth adding a test! Thanks @eitsupi for the catch. It's definitely related to this PR...the overlapping groups thing results in some very odd arguments to the mutate() embedded in group_by(), which results in all sorts of crazy things happening.

Is there any opposition to merging this?

Benchmark runs are scheduled for baseline = 16d0eb4 and contender = 9753a67. 9753a67 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️25.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.5% ⬆️0.23%] test-mac-arm
[Finished ⬇️12.5% ⬆️4.35%] ursa-i9-9960x
[Finished ⬇️11.16% ⬆️6.79%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 9753a672 ec2-t3-xlarge-us-east-2
[Finished] 9753a672 test-mac-arm
[Finished] 9753a672 ursa-i9-9960x
[Finished] 9753a672 ursa-thinkcentre-m75q
[Finished] 16d0eb4d ec2-t3-xlarge-us-east-2
[Finished] 16d0eb4d test-mac-arm
[Finished] 16d0eb4d ursa-i9-9960x
[Finished] 16d0eb4d 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

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x