[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #6625: Add hash support for PhysicalExpr and PhysicalSortExpr

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #6625:
URL: https://github.com/apache/arrow-datafusion/pull/6625#discussion_r1225823671


##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -132,6 +133,12 @@ impl PhysicalExpr for CastExpr {
             interval.cast_to(&cast_type, &self.cast_options)?,
         )])
     }
+
+    fn dyn_hash(&self, _state: &mut dyn Hasher) {
+        // `self.cast_options` doesn't support hashing
+        // Hence we cannot calculate `dyn_hash` for this type.
+        unimplemented!();

Review Comment:
   I filed https://github.com/apache/arrow-rs/pull/4395 to add this upstream
   
   
   Rather than `unimplmented` which will panic if this code is ever run (for eample if someone tries to put a CastExpr into a `HashSet`),  I think it is ok to hash the other fields while we wait for upstream.
   
   
   The requirements on hash https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq are 
   
   > In other words, if two keys are equal, their hashes must also be equal.
   
   Which I believe this implementation would satisfy:
   
   ```rust
   impl Hash for CastExpr {
       fn hash<H: Hasher>(&self, state: &mut H) {
           self.expr.hash(state);
           self.cast_type.hash(state);
           // Add self.cast_options when hash is available
           // https://github.com/apache/arrow-rs/pull/4395
       }
   }
   ```
   



##########
datafusion/physical-expr/src/equivalence.rs:
##########
@@ -39,7 +40,7 @@ pub struct EquivalenceProperties<T = Column> {
     schema: SchemaRef,
 }
 
-impl<T: PartialEq + Clone> EquivalenceProperties<T> {
+impl<T: Eq + Clone + Hash> EquivalenceProperties<T> {

Review Comment:
   ❤️ 



##########
datafusion/physical-expr/src/scalar_function.rs:
##########
@@ -162,6 +163,12 @@ impl PhysicalExpr for ScalarFunctionExpr {
             self.return_type(),
         )))
     }
+
+    fn dyn_hash(&self, _state: &mut dyn Hasher) {
+        // hashing for `self.fun` is not supported

Review Comment:
   likewise here can we create the `Hash` impl instead?



##########
datafusion/physical-expr/src/physical_expr.rs:
##########
@@ -104,6 +105,14 @@ pub trait PhysicalExpr: Send + Sync + Display + Debug + PartialEq<dyn Any> {
             "Not implemented for {self}"
         )))
     }
+
+    fn dyn_hash(&self, _state: &mut dyn Hasher);

Review Comment:
   I think we should add some comments here to help anyone who tries to implement `PhysicalExpr`
   
   Perhaps we can follow the model of the documentation on 
   
   https://docs.rs/datafusion-expr/26.0.0/datafusion_expr/logical_plan/trait.UserDefinedLogicalNode.html#tymethod.dyn_hash



##########
datafusion/physical-expr/src/expressions/in_list.rs:
##########
@@ -330,6 +331,12 @@ impl PhysicalExpr for InListExpr {
             self.static_filter.clone(),
         )))
     }
+
+    fn dyn_hash(&self, _state: &mut dyn Hasher) {

Review Comment:
   The same comment from above here applies -- I think a less precise hash function (with more collisions) is better than a runtime panic. Thus I think we should implement `Hash` for `static_filter`



##########
datafusion/physical-expr/src/expressions/cast.rs:
##########
@@ -132,6 +133,12 @@ impl PhysicalExpr for CastExpr {
             interval.cast_to(&cast_type, &self.cast_options)?,
         )])
     }
+
+    fn dyn_hash(&self, _state: &mut dyn Hasher) {
+        // `self.cast_options` doesn't support hashing
+        // Hence we cannot calculate `dyn_hash` for this type.
+        unimplemented!();

Review Comment:
   I filed https://github.com/apache/arrow-rs/pull/4395 to add this upstream
   
   
   Rather than `unimplmented` which will panic if this code is ever run (for eample if someone tries to put a CastExpr into a `HashSet`),  I think it is ok to hash the other fields while we wait for upstream.
   
   
   The requirements on hash https://doc.rust-lang.org/std/hash/trait.Hash.html#hash-and-eq are 
   
   > In other words, if two keys are equal, their hashes must also be equal.
   
   Which I believe this implementation would satisfy (though it will have some more hash collisions than when it would include `cast_options`):
   
   ```rust
   impl Hash for CastExpr {
       fn hash<H: Hasher>(&self, state: &mut H) {
           self.expr.hash(state);
           self.cast_type.hash(state);
           // Add self.cast_options when hash is available
           // https://github.com/apache/arrow-rs/pull/4395
       }
   }
   ```
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org