Address review: reset_cache guidance, provider note on the deprecated helper, ConfigOnlyContext invariant

Author: asolimando

datafusion/physical-optimizer/src/limit_pushdown.rs

@@ -158,6 +158,12 @@ struct LimitInfo {
 ///
 /// If a limit is encountered, a [`TreeNodeRecursion::Stop`] is returned. Otherwise,
 /// return a [`TreeNodeRecursion::Continue`].
+///
+/// Computes statistics with a new [`StatisticsContext`] that has no statistics
+/// providers. A context built from a statistics registry, as [`LimitPushdown`]
+/// uses, also consults the registered providers, so switching to
+/// [`pushdown_limit_helper_with_stats`] with such a context can change the
+/// result when providers are registered.
 #[deprecated(
     since = "56.0.0",
     note = "use `pushdown_limit_helper_with_stats` and share one `StatisticsContext` across calls"

datafusion/physical-optimizer/src/optimizer.rs

@@ -43,9 +43,10 @@ use datafusion_common::config::ConfigOptions;
 use datafusion_physical_plan::statistics::StatisticsContext;
 
 // Re-export from this module for backwards compatibility.
-pub use datafusion_session::with_statistics_context;
 pub use datafusion_session::{PhysicalOptimizerContext, PhysicalOptimizerRule};
 
+pub use datafusion_session::with_statistics_context;
+
 /// Simple context wrapping [`ConfigOptions`] for backward compatibility.
 ///
 /// This struct provides a minimal implementation of [`PhysicalOptimizerContext`]
@@ -62,6 +63,7 @@ impl<'a> ConfigOnlyContext<'a> {
     pub fn new(config: &'a ConfigOptions) -> Self {
         Self {
             config,
+            // No providers, matching `statistics_registry()`, which is `None`
             statistics_context: StatisticsContext::new(),
         }
     }

datafusion/physical-plan/src/statistics.rs

@@ -157,7 +157,8 @@ pub enum ChildStats {
 /// plan rewrites. Each entry holds a strong reference to the plan node it was
 /// computed for, so cached nodes (and their per-partition statistics) stay
 /// alive until [`Self::reset_cache`] is called or the context is dropped. Reset
-/// a long-lived context at a lifecycle boundary to bound its memory.
+/// a long-lived context at a lifecycle boundary to bound its memory (see
+/// [`Self::reset_cache`]).
 ///
 /// An optional [`StatisticsRegistry`] plugs providers into the walk: at each node
 /// they are consulted before the operator's built-in
@@ -199,8 +200,10 @@ impl StatisticsContext {
     /// Clears the memoization cache and releases its retained plan nodes.
     ///
     /// Resetting is optional for correctness: each cache entry retains the plan
-    /// node that supplied its pointer key. Use it to bound memory at a logical
-    /// lifecycle boundary, such as after an optimizer pass.
+    /// node that supplied its pointer key. Use it to bound memory at a
+    /// lifecycle boundary, such as the end of a query's physical optimization.
+    /// Do not reset a context that several optimizer rules share between those
+    /// rules: that discards the statistics that later rules would reuse.
     pub fn reset_cache(&self) {
         let mut cache = self.cache.lock();
         cache.statistics.clear();

docs/source/library-user-guide/upgrading/56.0.0.md

@@ -797,7 +797,10 @@ For the same reason,
 `datafusion_physical_optimizer::limit_pushdown::pushdown_limit_helper` is
 deprecated in favour of `pushdown_limit_helper_with_stats`, which takes the
 `StatisticsContext` from the caller. The deprecated form creates a new context
-on every call.
+on every call, without statistics providers. When providers are registered,
+switching to `pushdown_limit_helper_with_stats` with a context built from the
+statistics registry can change the result, because the providers are then
+consulted.
 
 ### `MergeIntoOp` requires the SQL-visible target qualifier