jayshrivastava commented on code in PR #21807:
URL: https://github.com/apache/datafusion/pull/21807#discussion_r3140843496
##########
datafusion/physical-expr/Cargo.toml:
##########
@@ -55,6 +55,7 @@ indexmap = { workspace = true }
itertools = { workspace = true, features = ["use_std"] }
parking_lot = { workspace = true }
petgraph = "0.8.3"
+rand = { workspace = true }
Review Comment:
This is a good question! I was thinking about a few alternatives 😄
- We could hash all the exprs in the `DynamicFilterPhysicalExpr` to get an
id. However, this wouldn't solve the shared `Inner` state linking problem. We
don't want this type of identifier - we want an identifier for the inner state
specifically.
- We could use the Arc address of the Inner struct, but it's a bit of a
smell to rely on pointer addresses - for example, IDs derived from Arc pointers
are only valid until the Arc is dropped. This is what the old code used and
something I used as well in initial versions of this PR
A rand u64 is not bad. Realistically, we just need to not have a collision
between distinct dynamic filters in a plan. I figure that the probability of
more than 2 or 3 distinct dynamic filters in a query must be very low already.
And the probability of 2 or 3 rand u64s colliding is negligible.
Lmk what you think!
--
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]
