-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(proto): Add protobuf serialization for HashExpr #19379
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
b2f4028
eaf4bf7
c2db472
2c89b17
6ccd80b
4012551
1cb6b1c
476415a
c68da54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -34,6 +34,36 @@ use datafusion_physical_expr_common::physical_expr::{ | |||||
|
|
||||||
| use crate::{hash_utils::create_hashes, joins::utils::JoinHashMapType}; | ||||||
|
|
||||||
| /// RandomState wrapper that preserves the seeds used to create it. | ||||||
| /// | ||||||
| /// This is needed because ahash's `RandomState` doesn't expose its seeds after creation, | ||||||
| /// but we need them for serialization (e.g., protobuf serde). | ||||||
| #[derive(Clone, Debug)] | ||||||
| pub struct SeededRandomState { | ||||||
| random_state: RandomState, | ||||||
| seeds: (u64, u64, u64, u64), | ||||||
| } | ||||||
|
|
||||||
| impl SeededRandomState { | ||||||
| /// Create a new SeededRandomState with the given seeds. | ||||||
| pub const fn with_seeds(k0: u64, k1: u64, k2: u64, k3: u64) -> Self { | ||||||
| Self { | ||||||
| random_state: RandomState::with_seeds(k0, k1, k2, k3), | ||||||
| seeds: (k0, k1, k2, k3), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Get the inner RandomState. | ||||||
| pub fn random_state(&self) -> &RandomState { | ||||||
| &self.random_state | ||||||
| } | ||||||
|
|
||||||
| /// Get the seeds used to create this RandomState. | ||||||
| pub fn seeds(&self) -> (u64, u64, u64, u64) { | ||||||
| self.seeds | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Physical expression that computes hash values for a set of columns | ||||||
| /// | ||||||
| /// This expression computes the hash of join key columns using a specific RandomState. | ||||||
|
|
@@ -45,8 +75,8 @@ use crate::{hash_utils::create_hashes, joins::utils::JoinHashMapType}; | |||||
| pub struct HashExpr { | ||||||
| /// Columns to hash | ||||||
| on_columns: Vec<PhysicalExprRef>, | ||||||
| /// Random state for hashing | ||||||
| random_state: RandomState, | ||||||
| /// Random state for hashing (with seeds preserved for serialization) | ||||||
| random_state: SeededRandomState, | ||||||
| /// Description for display | ||||||
| description: String, | ||||||
| } | ||||||
|
|
@@ -56,11 +86,11 @@ impl HashExpr { | |||||
| /// | ||||||
| /// # Arguments | ||||||
| /// * `on_columns` - Columns to hash | ||||||
| /// * `random_state` - RandomState for hashing | ||||||
| /// * `random_state` - SeededRandomState for hashing | ||||||
| /// * `description` - Description for debugging (e.g., "hash_repartition", "hash_join") | ||||||
| pub(super) fn new( | ||||||
| pub fn new( | ||||||
| on_columns: Vec<PhysicalExprRef>, | ||||||
| random_state: RandomState, | ||||||
| random_state: SeededRandomState, | ||||||
| description: String, | ||||||
| ) -> Self { | ||||||
| Self { | ||||||
|
|
@@ -69,6 +99,21 @@ impl HashExpr { | |||||
| description, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Get the columns being hashed. | ||||||
| pub fn on_columns(&self) -> &[PhysicalExprRef] { | ||||||
| &self.on_columns | ||||||
| } | ||||||
|
|
||||||
| /// Get the seeds used for hashing. | ||||||
| pub fn seeds(&self) -> (u64, u64, u64, u64) { | ||||||
| self.random_state.seeds() | ||||||
| } | ||||||
|
|
||||||
| /// Get the description. | ||||||
| pub fn description(&self) -> &str { | ||||||
| &self.description | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl std::fmt::Debug for HashExpr { | ||||||
|
|
@@ -87,12 +132,15 @@ impl Hash for HashExpr { | |||||
| fn hash<H: std::hash::Hasher>(&self, state: &mut H) { | ||||||
| self.on_columns.dyn_hash(state); | ||||||
| self.description.hash(state); | ||||||
| self.seeds().hash(state); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl PartialEq for HashExpr { | ||||||
| fn eq(&self, other: &Self) -> bool { | ||||||
| self.on_columns == other.on_columns && self.description == other.description | ||||||
| self.on_columns == other.on_columns | ||||||
| && self.description == other.description | ||||||
| && self.seeds() == other.seeds() | ||||||
|
Comment on lines
+136
to
+144
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think there was also a bug lurking here where expressions would erroneously compare equal even if they had different |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -147,7 +195,11 @@ impl PhysicalExpr for HashExpr { | |||||
|
|
||||||
| // Compute hashes | ||||||
| let mut hashes_buffer = vec![0; num_rows]; | ||||||
| create_hashes(&keys_values, &self.random_state, &mut hashes_buffer)?; | ||||||
| create_hashes( | ||||||
| &keys_values, | ||||||
| self.random_state.random_state(), | ||||||
| &mut hashes_buffer, | ||||||
| )?; | ||||||
|
|
||||||
| Ok(ColumnarValue::Array(Arc::new(UInt64Array::from( | ||||||
| hashes_buffer, | ||||||
|
|
@@ -211,8 +263,7 @@ impl Hash for HashTableLookupExpr { | |||||
|
|
||||||
| impl PartialEq for HashTableLookupExpr { | ||||||
| fn eq(&self, other: &Self) -> bool { | ||||||
| Arc::ptr_eq(&self.hash_expr, &other.hash_expr) | ||||||
| && self.description == other.description | ||||||
| self.hash_expr.dyn_eq(&other.hash_expr) && self.description == other.description | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be:
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes good catch, I've fixed and added tests: 4012551 |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
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 like the fact that having this
SeededRandomStatestruct is an explicit indicator that the underlayingRandomStateis not completely random.