asolimando commented on code in PR #26094:
URL: https://github.com/apache/datafusion/pull/26094#discussion_r4219502909
##########
datafusion/physical-plan/src/statistics.rs:
##########
@@ -203,7 +202,7 @@ impl StatisticsContext {
/// node that supplied its pointer key. Use it to bound memory at a logical
/// lifecycle boundary, such as after an optimizer pass.
pub fn reset_cache(&self) {
- let mut cache = self.cache.borrow_mut();
+ let mut cache = self.cache.lock();
Review Comment:
Good catch, thanks! Updated: `reset_cache` now says to reset at a lifecycle
boundary such as the end of a query's physical optimization, and not between
rules that share a context.
##########
datafusion/physical-optimizer/src/limit_pushdown.rs:
##########
@@ -145,9 +158,27 @@ struct LimitInfo {
///
/// If a limit is encountered, a [`TreeNodeRecursion::Stop`] is returned.
Otherwise,
/// return a [`TreeNodeRecursion::Continue`].
+#[deprecated(
+ since = "56.0.0",
+ note = "use `pushdown_limit_helper_with_stats` and share one
`StatisticsContext` across calls"
Review Comment:
Agreed, added a paragraph to the deprecated function's doc and a sentence to
the upgrade guide: with a context built from the registry, the registered
providers are consulted, so the result can change when providers are registered.
##########
datafusion/physical-optimizer/src/optimizer.rs:
##########
@@ -40,30 +40,41 @@ use crate::limit_pushdown_past_window::LimitPushPastWindows;
use crate::pushdown_sort::PushdownSort;
use crate::window_topn::WindowTopN;
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};
/// Simple context wrapping [`ConfigOptions`] for backward compatibility.
///
/// This struct provides a minimal implementation of
[`PhysicalOptimizerContext`]
-/// that only supplies configuration options. Used when no statistics registry
-/// is available or needed.
+/// that supplies configuration options and a [`StatisticsContext`] without a
+/// statistics registry. Used when no statistics registry is available or
+/// needed.
pub struct ConfigOnlyContext<'a> {
config: &'a ConfigOptions,
+ statistics_context: StatisticsContext,
}
impl<'a> ConfigOnlyContext<'a> {
/// Create a new context wrapping the given config options.
pub fn new(config: &'a ConfigOptions) -> Self {
- Self { config }
+ Self {
+ config,
+ statistics_context: StatisticsContext::new(),
Review Comment:
Added a one-line comment stating the invariant (no providers, matching
`statistics_registry()` returning `None`).
--
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]
