GH-45373: [R] summarize after arrange fails

[thisisnic]

Rationale for this change

arrange() then summarise() failed

What changes are included in this PR?

Reset ordering vars before summarise like dplyr etc do anyway

Are these changes tested?

Yep

Are there any user-facing changes?

No

• GitHub Issue: [R] summarize after arrange fails #45373

[github-actions]

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

[Copilot]

🔵 Needs a closer look

Pending head()/tail() operations can still aggregate the wrong rows; apply slicing before aggregation and add regression tests.

Pull request overview

Fixes deferred arrange() metadata issues in R dplyr aggregation pipelines.

Changes:

• Clears stale sorting before summarize() and aggregate-backed mutate().
• Adds regression tests for affected pipelines.

File summaries

File	Description
r/tests/testthat/test-dplyr-summarize.R	Adds summarize regression tests.
r/tests/testthat/test-dplyr-mutate.R	Adds aggregate-mutate regression tests.
r/R/dplyr-summarize.R	Resets sorting before summarization.
r/R/dplyr-mutate.R	Resets sorting in aggregation subqueries.

Review details

Suppressed comments (2)

r/R/dplyr-mutate.R:79

• Clearing the sort here has the same incorrect interaction with a pending head()/tail() on the input. For example, arrange(int) |> head(3) |> mutate(avg = mean(int, na.rm = TRUE)) computes avg over the full input in the aggregation side of the join, then returns only three rows, rather than computing it over the three selected rows. Please apply the slice before aggregating (or reject this combination) and add a regression test.

      agg_query$arrange_vars <- list()
      agg_query$arrange_desc <- logical()

r/R/dplyr-summarize.R:92

• This also drops the pending sort that may be needed by a following head()/tail() on the input. For example, arrange(int) |> head(3) |> summarize(total = sum(int, na.rm = TRUE)) now aggregates all rows and only then applies the fetch to the one-row aggregate, instead of aggregating the selected three rows; before this change it failed on the missing sort key. Please apply the slice before aggregating (or reject this combination) and add a regression test.

  .data$arrange_vars <- list()
  .data$arrange_desc <- logical()

• Files reviewed: 4/4 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.

[Copilot]

🟢 Approval recommended

No unresolved review issues remain, and regression tests are included.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The row-limit regression tests do not verify that sorting is honored because their input is already sorted.

Review details

Suppressed comments (1)

r/tests/testthat/test-dplyr-summarize.R:1361

• This head(3) regression case does not distinguish sorted from unsorted input: tbl is example_data, whose int values are already ascending, so taking the first three rows gives the same result even if arrange() is ignored. Use a descending or shuffled input here (and in the analogous mutate test) so the assertion actually verifies that the row limit is applied after sorting.

    .input |>
      arrange(int) |>
      head(3) |>

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

Two regression assertions do not adequately detect dropped sorting or limiting behavior.

Review details

Suppressed comments (2)

r/tests/testthat/test-dplyr-mutate.R:794

• This assertion does not distinguish the sorted head(3) rows from the full input: with int = 1:3, NA, 5:10, max(int) is 10 in both cases. A regression that drops the limit/sort while building the aggregation query would still pass, so use an aggregate whose result changes (for example min_int, which should be 8 for the descending top three versus 1 for the full input).

      mutate(max_int = max(int, na.rm = TRUE)) |>

r/tests/testthat/test-dplyr-summarize.R:1362

• This assertion does not distinguish the sorted head(3) rows from the full input: with int = 1:3, NA, 5:10, max(int) is 10 in both cases. A regression that drops the limit/sort while building the aggregation query would still pass, so use an aggregate whose result changes (for example min_int, which should be 8 for the descending top three versus 1 for the full input).

      summarize(max_int = max(int, na.rm = TRUE)) |>

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

No unresolved review issues were identified.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jonkeane]

Were these then just referenced elsewhere without checking if they exist? Was that the bug? Wild!

[jonkeane]

Thanks for this