fix: DynamicFilterPhysicalExpr violates Hash/Eq contract

[kumarUjjawal]

Which issue does this PR close?

• Closes DynamicFilterPhysicalExpr violates Hash/Eq contract #19641.

Rationale for this change

DynamicFilterPhysicalExpr violates the Hash/Eq contract because the Hash and PartialEq implementations each call self.current() which acquires separate RwLock::read() locks. This allows the underlying expression to change between
hash() and eq() calls via update(), causing:

• HashMap key instability (keys "disappear" after update)
• Potential infinite loops during HashMap operations
• Corrupted HashMap state during concurrent access

What changes are included in this PR?

Replaced content-based Hash/Eq with identity-based implementations:

• Hash: Uses Arc::as_ptr(&self.inner) instead of hashing the mutable expression content
• PartialEq: Uses Arc::ptr_eq(&self.inner) instead of comparing expression content via locks

Are these changes tested?

Yes

Are there any user-facing changes?

[kumarUjjawal]

cc @adriangb

[adriangb]

Thanks for working on this.

Were there any alternatives considered? I've thought about it a little bit and think this is probably the best path forward, but maybe there are other alternatives; it would be good to document why we chose this option. At least one would be to make Hash panic and say you need to call snapshot_physical_expr to be explicit. Wdyt?

[kumarUjjawal]

Thanks for the feedback, I did some analysis before implementation to best of my understanding;

Approach	Pros	Cons
Identity-based (chosen)	Stable hash, no breaking change, correct semantics for filter identity	Different instances with the same expression aren’t equal
Panic in Hash	Explicit, forces use of snapshot_physical_expr()	Breaking change — any HashSet<Arc<dyn PhysicalExpr>> containing dynamic filters would panic
Include generation in hash	Content-aware	Still violates the contract — race between hash() and eq()
Document only	No code change	Silent runtime bugs remain

Current Implementation:

• PhysicalExpr requires Hash Generic code (e.g., equivalence classes) uses HashMap<Arc<dyn PhysicalExpr>, _>. Panicking would break this for any tree containing DynamicFilterPhysicalExpr.

• Two dynamic filters with different inner Arcs are different filters (independent update lifecycles), even if they have the same expression at some moment.

• with_new_children() does the right thing Filters created via tree transformation share the inner value and compare equal, which is exactly what filter pushdown expects.

The panic approach is reasonable but would be a breaking change(I think). Happy to discuss further if you think the explicitness outweighs the compatibility benefit.

[adriangb]

Sound good to me. Could we look for any potential call sites and update them to use snapshot_physical_expr if they care about differentiating between two DynamicFilterPhysicalExpr? E.g. in #19639 I needed to differentiate them because if a DynamicFilterPhysicalExpr updates it's expression I want to treat it differently than the previous version.

[adriangb]

I wonder if we could somehow use Inner::generation to our advantage? I think it runs into the same issues as structural comparisons...

[adriangb]

I'm approving because this seems like the best choice and is better than the status quo. I imagine there may be uses where a different behavior is desired, we've tried to think through them but can't pretend to have covered them all. For future readers: if a different behavior is required please comment on this PR or open a new issue.

[adriangb]

(let's wait a day before merging this)

[alamb]

I think we should include this in datafusion-52 so I added it to the list on

• Release DataFusion 52.0.0 (Dec 2025 / Jan 2026) #18566

[alamb]

Proposed backport:

• [branch-52] fix: DynamicFilterPhysicalExpr violates Hash/Eq contract (#19659) #19705