Skip to content

Commit f0a3579

Browse files
adriangbclaude
andcommitted
perf: Cache remapped expression in DynamicFilterPhysicalExpr::current()
`current()` calls `remap_children()` which does a full `transform_up()` tree walk on every invocation. This is called per-batch in `evaluate()`, `snapshot()`, etc. When the inner expression hasn't changed (the common case — updates are infrequent relative to evaluations), this tree walk is pure redundant work. Cache the remapped result per-instance using `Arc::ptr_eq` on the source expression for invalidation. When `update()` replaces `inner.expr` with a new `Arc`, the pointer naturally changes and the cache invalidates. Co-Authored-By: Claude Opus 4.6 <[email protected]>
1 parent 80485d7 commit f0a3579

1 file changed

Lines changed: 31 additions & 2 deletions

File tree

‎datafusion/physical-expr/src/expressions/dynamic_filters.rs‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,10 @@ pub struct DynamicFilterPhysicalExpr {
6969
inner: Arc<RwLock<Inner>>,
7070
/// Broadcasts filter state (updates and completion) to all waiters.
7171
state_watch: watch::Sender<FilterState>,
72+
/// Cached result of `remap_children` to avoid redundant tree walks in `current()`.
73+
/// Stores (source_expr_from_inner, remapped_result). Invalidated when the source
74+
/// pointer changes (i.e., after `update()` replaces the inner expression).
75+
cached_current: RwLock<Option<(Arc<dyn PhysicalExpr>, Arc<dyn PhysicalExpr>)>>,
7276
/// For testing purposes track the data type and nullability to make sure they don't change.
7377
/// If they do, there's a bug in the implementation.
7478
/// But this can have overhead in production, so it's only included in our tests.
@@ -175,6 +179,7 @@ impl DynamicFilterPhysicalExpr {
175179
remapped_children: None, // Initially no remapped children
176180
inner: Arc::new(RwLock::new(Inner::new(inner))),
177181
state_watch,
182+
cached_current: RwLock::new(None),
178183
data_type: Arc::new(RwLock::new(None)),
179184
nullable: Arc::new(RwLock::new(None)),
180185
}
@@ -218,8 +223,31 @@ impl DynamicFilterPhysicalExpr {
218223
/// This will return the current expression with any children
219224
/// remapped to match calls to [`PhysicalExpr::with_new_children`].
220225
pub fn current(&self) -> Result<Arc<dyn PhysicalExpr>> {
221-
let expr = Arc::clone(self.inner.read().expr());
222-
Self::remap_children(&self.children, self.remapped_children.as_ref(), expr)
226+
let source = Arc::clone(self.inner.read().expr());
227+
228+
// Fast path: no remapping needed
229+
if self.remapped_children.is_none() {
230+
return Ok(source);
231+
}
232+
233+
// Check cache: if source hasn't changed, return cached remap
234+
{
235+
let cache = self.cached_current.read();
236+
if let Some((cached_source, cached_result)) = cache.as_ref() {
237+
if Arc::ptr_eq(cached_source, &source) {
238+
return Ok(Arc::clone(cached_result));
239+
}
240+
}
241+
}
242+
243+
// Cache miss: recompute and cache
244+
let remapped = Self::remap_children(
245+
&self.children,
246+
self.remapped_children.as_ref(),
247+
Arc::clone(&source),
248+
)?;
249+
*self.cached_current.write() = Some((source, Arc::clone(&remapped)));
250+
Ok(remapped)
223251
}
224252

225253
/// Update the current expression and notify all waiters.
@@ -370,6 +398,7 @@ impl PhysicalExpr for DynamicFilterPhysicalExpr {
370398
remapped_children: Some(children),
371399
inner: Arc::clone(&self.inner),
372400
state_watch: self.state_watch.clone(),
401+
cached_current: RwLock::new(None),
373402
data_type: Arc::clone(&self.data_type),
374403
nullable: Arc::clone(&self.nullable),
375404
}))

0 commit comments

Comments
 (0)