zhuqi-lucas commented on code in PR #26094:
URL: https://github.com/apache/datafusion/pull/26094#discussion_r4219244228
##########
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:
The doc above this still says to use `reset_cache` "at a logical lifecycle
boundary, such as after an optimizer pass". That advice was fine when each rule
owned its context, but now one context spans every rule of an
`optimize_physical_plan` run, so resetting after a single pass throws away
exactly what this PR set out to keep.
Nothing in-tree calls it outside tests, so this is docs-only — but a custom
rule following the current wording would quietly undo the win.
##########
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:
Minor: this hardcodes the no-registry constructor while
`statistics_context()` returns `Some(..)`, which makes
`with_statistics_context` take the shared branch and skip the registry fallback
entirely.
Correct today because `ConfigOnlyContext::statistics_registry()` is `None`,
but the two are now coupled — if this type ever gains a registry, the context
built here would silently ignore it. A one-line comment stating the invariant
would be enough.
##########
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:
Keeping `StatisticsContext::new()` here is right for back-compat — it
preserves the old no-provider behaviour for existing callers.
But that's a second difference the note doesn't mention: migrating to
`pushdown_limit_helper_with_stats` (via a context from the registry) also
changes *which* providers are consulted, not just whether the cache is shared.
Might be worth a few words so callers with registered providers aren't
surprised by a result change.
--
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]
