8000 GH-46652: [Python][Docs] Update language for row_group_size parameter by amoeba · Pull Request #46653 · apache/arrow · GitHub
[go: up one dir, main page]

Skip to content

GH-46652: [Python][Docs] Update language for row_group_size parameter #46653

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.

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

Conversation

amoeba
Copy link
Member
@amoeba amoeba commented May 31, 2025

Rationale for this change

The docstrings for row_group_size could be clearer both in terms of (1) whether the value is rows instead of byte size and (2) use of unit prefixes. See #46652.

My idea here was that just saying "64 * 1024 * 1024" is probably more easily understood than using Mi (mebi). The existing text may be just fine so I'm happy to close this if others like how it reads now.

What changes are included in this PR?

  • Updated language in docstrings for row_group_size
  • Add missing , default None to docstring for top-level write_table

Are these changes tested?

No.

Are there any user-facing changes?

No.

@amoeba amoeba requested review from AlenkaF, raulcd and rok as code owners May 31, 2025 00:55
Copy link

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

Copy link

⚠️ GitHub issue #46652 has no components, please add labels for components.

@amoeba
Copy link
Member Author
amoeba commented May 31, 2025

I think the two current CI failures are unrelated. archery lint --python locally shows some issues but none for the affected code.

@amoeba amoeba requested a review from westonpace May 31, 2025 02:54
Copy link

⚠️ GitHub issue #46652 has no components, please add labels for components.

Copy link
Member
@AlenkaF AlenkaF left a comment

Choose a reason for hiding this comment

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

I think this is a good change — it helps avoid confusion, which I also struggled with.

@github-actions github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 2, 2025
@amoeba amoeba requested a review from pitrou June 3, 2025 17:33
Copy link
Member
@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

Agreed with @AlenkaF !

@AlenkaF AlenkaF merged commit 832bfa1 into apache:main Jun 4, 2025
14 of 16 checks passed
@AlenkaF AlenkaF removed the awaiting committer review Awaiting committer review label Jun 4, 2025
@amoeba
Copy link
Member Author
amoeba commented Jun 4, 2025

Thanks @AlenkaF @pitrou.

Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 832bfa1.

There were 72 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 42 possible false positives for unstable benchmarks that are known to sometimes produce them.

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

Successfully merging this pull request may close these issues.

3 participants
0