Skip to content

Commit 8041f43

Browse files
committed
Address Copilot review: guard cache write against older-generation clobber
A slow `current()` call that observed an older `inner` generation could finish after a concurrent caller has published a newer entry, and its unconditional write would replace that newer entry with a stale one. Semantics were still correct (later readers see the mismatch and recompute), but subsequent generation-matched calls would miss the cache and redo the remap unnecessarily. Guard the write: only publish if the observed generation is at least as fresh as whatever is currently cached.
1 parent 3f4cee9 commit 8041f43

1 file changed

Lines changed: 16 additions & 4 deletions

File tree

  • datafusion/physical-expr/src/expressions/dynamic_filters

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

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -269,10 +269,22 @@ impl DynamicFilterPhysicalExpr {
269269
// Slow path: (re)compute the remap and store it under a write lock.
270270
let remapped =
271271
Self::remap_children(&self.children, self.remapped_children.as_ref(), expr)?;
272-
// A concurrent thread may have populated the cache with the same
273-
// generation; either wins the write is fine — both are semantically
274-
// equivalent because the inner generation hasn't changed.
275-
*self.current_cache.write() = Some((generation, Arc::clone(&remapped)));
272+
// Only publish our result if it is at least as fresh as whatever is
273+
// currently cached. Without this guard a slow computation that
274+
// observed an older `inner` could clobber a newer entry that a
275+
// concurrent caller has already published (see #23532 review), which
276+
// would force subsequent readers to redo the remap for the newer
277+
// generation.
278+
{
279+
let mut cache = self.current_cache.write();
280+
let should_write = match cache.as_ref() {
281+
Some((cached_gen, _)) => generation >= *cached_gen,
282+
None => true,
283+
};
284+
if should_write {
285+
*cache = Some((generation, Arc::clone(&remapped)));
286+
}
287+
}
276288
Ok(remapped)
277289
}
278290

0 commit comments

Comments
 (0)