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.

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

refactor: add get_available_parallelism function #13595

Merged
merged 2 commits into from
Nov 29, 2024
Merged

Conversation

alan910127
Copy link
Contributor

Which issue does this PR close?

Closes #13591.

Rationale for this change

In #13579 (review), it was suggested that the repeated use of std::thread::available_parallelism with fallback logic could be refactored for clarity and to reduce duplication. Extracting this logic into a utility function improves code readability and reduces the likelihood of errors.

What changes are included in this PR?

This PR introduces a new utility function:

pub fn get_available_parallelism() -> usize;

It replaces the following repeated code snippet:

std::thread::available_parallelism().unwrap_or(NonNull::new(1).unwrap()).get()

with a call to the new function:

get_available_parallelism()

Are these changes tested?

No tests are added for this change as it is a straightforward refactor of existing functionality. However, if reviewers feel this requires testing, I am happy to add them.

Are there any user-facing changes?

No.

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Nov 28, 2024
@findepi
Copy link
Member

findepi commented Nov 28, 2024

The clippy job failed. This is probably nothing wrong with this PR, see #13597.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @alan910127 for your first contribution.
Please take care on clippy. I feel the PR is good.

@alan910127
Copy link
Contributor Author

alan910127 commented Nov 28, 2024

Hi @comphead, thanks for your review! I just checked the pipeline logs, the clippy errors are not related to my changes. Perhaps it's what @findepi mentioned in the previous comment.

@comphead
Copy link
Contributor

@alan910127 please rebase from the latest main

@alan910127
Copy link
Contributor Author

@comphead Rebased.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @alan910127

@comphead
Copy link
Contributor

Since this is a first time contribution I'll be waiting for another review before merging it in

Copy link
Member

@jonahgao jonahgao left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @alan910127 . I merged main, so that this PR can be checked by the new version of clippy.

@jonahgao jonahgao merged commit eca0e07 into apache:main Nov 29, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move available_parallelism() into utility function
4 participants