asolimando commented on code in PR #26094:
URL: https://github.com/apache/datafusion/pull/26094#discussion_r4207349745
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -173,7 +172,7 @@ pub enum ChildStats {
/// [`Self::compute_extended`] observes extensions; [`Self::compute`] returns
core
/// [`Statistics`] only.
pub struct StatisticsContext {
- cache: Rc<RefCell<StatsCache>>,
+ cache: Mutex<StatsCache>,
Review Comment:
Note for reviewers: the `Mutex` is needed because `PhysicalOptimizerContext:
Send + Sync`, and the shared context is held inside it and returned as
`&StatisticsContext`. `RefCell` is not `Sync` (and the `Rc` was never cloned,
so it was dropped rather than replaced).
The optimizer uses the context from one thread, so the lock is never
contended, and it is briefly held (single map lookup/insert), never across the
recursive walk. The `compute_statistics` benchmark shows no measurable
difference from `RefCell` (all cases within ±4% of `main`, in both directions).
Alternatives I considered: a `RefCell` restricted to the creating thread
(needs `unsafe impl Sync`), a thread-local cache (hidden global state), or
removing `Sync` from `PhysicalOptimizerContext` (a breaking change). None of
them seemed worth it for a lock that costs nothing measurable.
--
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]
