-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Move hash utils to common #7684
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @jayzhan211
I double checked and this change adds no new dependencies:
https://crates.io/crates/half/reverse_dependencies is already a dependency on arrow as is ahash: https://crates.io/crates/ahash/reverse_dependencies?page=3
@@ -28,7 +28,6 @@ pub mod equivalence; | |||
pub mod execution_props; | |||
pub mod expressions; | |||
pub mod functions; | |||
pub mod hash_utils; |
There was a problem hiding this 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 technically a breaking change if someone used hash_utils
directly in their code.
Perhaps we could leave a pointer like
pub mod hash_utils; | |
// backwards compatibility | |
pub use datafusion_common::hash_util; |
But I don't think that is critical
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
954dfd7
to
8d5aa69
Compare
how to re-run CI without code changes? |
You can go to the job and click rerun: https://github.com/apache/arrow-datafusion/actions/runs/6345797930/job/17238371168?pr=7684 |
It must be related to permissions. Another way to do so is to merge up from main ( |
* move hash utils to common Signed-off-by: jayzhan211 <jayzhan211@gmail.com> * support backward compatibility Signed-off-by: jayzhan211 <jayzhan211@gmail.com> --------- Signed-off-by: jayzhan211 <jayzhan211@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Which issue does this PR close?
Closes #.
Rationale for this change
While working on #7629, I found
create_hashes
is placed to physical-expr.Hash utils only import functions from arrow, std, and df-common. It seems it should be placed to df-common not physical-expr.
What changes are included in this PR?
We can have hash_utils from df-common.
Are these changes tested?
Nope, only import is modified, no need to add additional tests.
Are there any user-facing changes?