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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
