Skip to content

GH-40742: [R] fix max_rows_per_group must be a positive number#49709

Merged
thisisnic merged 3 commits intoapache:mainfrom
thisisnic:GH-40742-max-rows-per-file
May 5, 2026
Merged

GH-40742: [R] fix max_rows_per_group must be a positive number#49709
thisisnic merged 3 commits intoapache:mainfrom
thisisnic:GH-40742-max-rows-per-file

Conversation

@thisisnic
Copy link
Copy Markdown
Member

@thisisnic thisisnic commented Apr 11, 2026

Rationale for this change

Error message due to faulty checking

What changes are included in this PR?

Change checking circumstances

Are these changes tested?

Yes

Are there any user-facing changes?

No

AI usage

Used Claude to generate this - will double check before marking ready for review

@github-actions
Copy link
Copy Markdown

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

@thisisnic thisisnic marked this pull request as ready for review April 30, 2026 11:29
@thisisnic thisisnic requested a review from jonkeane as a code owner April 30, 2026 11:29
Copy link
Copy Markdown
Member

@jonkeane jonkeane left a comment

Choose a reason for hiding this comment

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

Thanks for this, one question to make sure my read is accurate, but looks good + easy test!

Comment thread r/R/dataset-write.R
if (
!missing(max_rows_per_file) &&
missing(max_rows_per_group) &&
max_rows_per_file > 0 &&
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line max_rows_per_file > 0 is the one condition that was change, yeah? The rest is whitespace?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup!

@github-actions github-actions Bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels May 5, 2026
@thisisnic thisisnic merged commit 9a0afd9 into apache:main May 5, 2026
19 checks passed
@thisisnic thisisnic removed the awaiting merge Awaiting merge label May 5, 2026
@github-actions github-actions Bot added the awaiting changes Awaiting changes label May 5, 2026
@conbench-apache-arrow
Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 9a0afd9.

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants