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

Minor: change visibility of hash join utils #13893

Merged
merged 1 commit into from
Dec 24, 2024

Conversation

berkaysynnada
Copy link
Contributor

@berkaysynnada berkaysynnada commented Dec 24, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

These functions can be used in other modules across the crate, so I'm relaxing their accessibility.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Dec 24, 2024
Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

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

LGTM -- we indeed find these utility functions useful in our fork, which may be the case for others too. Instead of making everyone duplicate the logic, let's make them crate-accessible.

@ozankabak ozankabak merged commit 901a094 into apache:main Dec 24, 2024
25 checks passed
@@ -176,7 +176,7 @@ fn swap_join_projection(
/// This function swaps the inputs of the given join operator.
/// This function is public so other downstream projects can use it
/// to construct `HashJoinExec` with right side as the build side.
pub fn swap_hash_join(
pub(crate) fn swap_hash_join(
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is actually a downgrade of visibility -- it used to be pub but now is pub(crate) meaning that comet can't use it: #13898

I will make a PR to fix this

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry for that. I guess we hastily thought pub(crate) would be sufficient but in Comet's case they need the full pub.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries -- it was easily fixed

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

Successfully merging this pull request may close these issues.

3 participants