Copilot commented on code in PR #23532:
URL: https://github.com/apache/datafusion/pull/23532#discussion_r3570973757
##########
datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs:
##########
@@ -232,9 +247,33 @@ impl DynamicFilterPhysicalExpr {
/// Get the current expression.
/// This will return the current expression with any children
/// remapped to match calls to [`PhysicalExpr::with_new_children`].
+ ///
+ /// Called per batch on the RowFilter path (via
+ /// [`PhysicalExpr::evaluate`]). The remap walk is O(tree size) and, for
+ /// dynamic filters that carry a large `InListExpr` (join key IN list),
+ /// dominated by `InListExpr::with_new_children` cloning the whole list.
+ /// The inner generation only changes when [`Self::update`] fires, so we
+ /// cache the remapped expression per generation and return it directly
+ /// on subsequent per-batch calls.
pub fn current(&self) -> Result<Arc<dyn PhysicalExpr>> {
- let expr = Arc::clone(self.inner.read().expr());
- Self::remap_children(&self.children, self.remapped_children.as_ref(),
expr)
+ // Fast path: cache hit for the current generation.
+ let (expr, generation) = {
+ let inner = self.inner.read();
+ (Arc::clone(inner.expr()), inner.generation)
+ };
+ if let Some((cached_gen, cached_expr)) =
self.current_cache.read().as_ref()
+ && *cached_gen == generation
+ {
+ return Ok(Arc::clone(cached_expr));
+ }
+ // Slow path: (re)compute the remap and store it under a write lock.
+ let remapped =
+ Self::remap_children(&self.children,
self.remapped_children.as_ref(), expr)?;
+ // A concurrent thread may have populated the cache with the same
+ // generation; either wins the write is fine — both are semantically
+ // equivalent because the inner generation hasn't changed.
+ *self.current_cache.write() = Some((generation,
Arc::clone(&remapped)));
Review Comment:
`current()` can race with a concurrent `update()`: a call that observed an
older `generation` may finish later and overwrite the cache entry for a newer
generation. This doesn’t break correctness (generation mismatch forces a
recompute) but it can defeat the optimization and keep stale expressions alive
longer. Consider guarding the write so an older generation never overwrites a
newer cached generation.
--
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]
