asolimando commented on code in PR #23651:
URL: https://github.com/apache/datafusion/pull/23651#discussion_r3605458984
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -37,34 +50,15 @@ use std::sync::Arc;
/// The pointer-based key is safe within a single synchronous walk:
/// all `Arc<dyn ExecutionPlan>` nodes are held by the plan tree for
/// the duration of the walk, so addresses cannot be reused.
+///
+/// Core statistics and provider extensions are cached separately: the
+/// `statistics` map is the hot path (populated on every walk); the
`extensions`
+/// map is populated only when a provider returns non-empty extensions, so a
walk
+/// with no providers never touches it.
#[derive(Debug, Default)]
-struct StatsCache(HashMap<(usize, Option<usize>), Arc<Statistics>>);
-
-impl StatsCache {
- fn get(
- &self,
- plan: &dyn ExecutionPlan,
- partition: Option<usize>,
- ) -> Option<&Arc<Statistics>> {
- let key = (
- plan as *const dyn ExecutionPlan as *const () as usize,
- partition,
- );
- self.0.get(&key)
- }
-
- fn insert(
- &mut self,
- plan: &dyn ExecutionPlan,
- partition: Option<usize>,
- stats: Arc<Statistics>,
- ) {
- let key = (
- plan as *const dyn ExecutionPlan as *const () as usize,
- partition,
- );
- self.0.insert(key, stats);
- }
+struct StatsCache {
+ statistics: HashMap<CacheKey, Arc<Statistics>>,
+ extensions: HashMap<CacheKey, Extensions>,
Review Comment:
I kept
https://github.com/apache/datafusion/pull/23651/changes#diff-149914d58cc354ddd4d84e283de4f8f07f3295b39137d850b3344448d4f6dc7a
separated on purpose in case reviewers prefer to drop it from this PR.
The reason to have two caches is that caching on `ExtendedStatistics` at
every node regressed the `compute_statistics` benchmark ~6% to 28% (median
~13%) on the no-provider path (a wrapper allocation per node).
The committed version keeps core `Statistics` in the walk and holds
`Extensions` in a side map, populated only when a provider returns some.
Benchmark parity when no providers are registered, extensions still reach
parent-node providers.
I think this commit is worth keeping for feature parity w.r.t.
`StatisticsRegistry` as existing today.
--
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]
