adriangb commented on code in PR #20416:
URL: https://github.com/apache/datafusion/pull/20416#discussion_r2843240928
##########
datafusion/physical-expr/src/expressions/dynamic_filters.rs:
##########
@@ -88,6 +88,69 @@ struct Inner {
is_complete: bool,
}
+/// An atomic snapshot of a [`DynamicFilterPhysicalExpr`] used to reconstruct
the expression during
+/// serialization / deserialization.
+pub struct DynamicFilterSnapshot {
Review Comment:
It seems like we end up exposing all of the internals anyway: if there was a
way to return a `dyn Serializable` it'd be one thing, but the proto machinery
still needs to know all of the fields to serialize.
##########
datafusion/physical-expr/src/expressions/dynamic_filters.rs:
##########
@@ -327,6 +468,14 @@ impl DynamicFilterPhysicalExpr {
Arc::strong_count(self) > 1 || Arc::strong_count(&self.inner) > 1
}
+ /// Returns a unique identifier for the inner shared state.
+ ///
+ /// Useful for checking if two [`Arc<PhysicalExpr>`] with the same
+ /// underlying [`DynamicFilterPhysicalExpr`] are the same.
+ pub fn inner_id(&self) -> u64 {
+ Arc::as_ptr(&self.inner) as *const () as u64
Review Comment:
I also agree that it's quite hacky to use pointer addresses.
The main reason I went down this path is that (I hoped) it "just worked" for
all expressions and avoided special casing `DynamicFilterPhysicalExpr` or
polluting the APIs.
If we were to add a unique identifier it would be unfortunate if every
`PhysicalExpr` implementation had to do something to handle this change, most
of them don't have internal mutability.
Maybe a compromise / first step would be:
```rust
trait PhysicalExpr {
fn expr_id(Arc<Self>) -> u64 {
// do pointer shennanigans
}
}
impl PhysicalExpr for DynamicFilterPhysicalExpr {
fn expr_id(Arc<Self>) -> u64 {
self.id // some randomly generated integer
}
}
```
That way all physical expressions can be deduplicated / uniquely identified?
Alternatively the method could be `Option<u64>` and the default impl returns
`None` -> no deduplication for this expression.
##########
datafusion/physical-expr/src/expressions/dynamic_filters.rs:
##########
@@ -88,6 +88,69 @@ struct Inner {
is_complete: bool,
}
+/// An atomic snapshot of a [`DynamicFilterPhysicalExpr`] used to reconstruct
the expression during
+/// serialization / deserialization.
+pub struct DynamicFilterSnapshot {
Review Comment:
I also think the name snapshot makes it sounds like this is what would be
returned from `PhysicalExpr::snapshot`, but that is not the case.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
