Copilot commented on code in PR #18739: URL: https://github.com/apache/datafusion/pull/18739#discussion_r2552644385
########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -49,12 +89,49 @@ use datafusion_physical_plan::ExecutionPlan; /// /// [`SessionState::add_physical_optimizer_rule`]: https://docs.rs/datafusion/latest/datafusion/execution/session_state/struct.SessionState.html#method.add_physical_optimizer_rule pub trait PhysicalOptimizerRule: Debug { + /// Rewrite `plan` to an optimized form with additional context + /// + /// This is the preferred method for implementing optimization rules as it + /// provides access to the full optimizer context including session configuration, + /// runtime environment, and extensions. Review Comment: The documentation mentions 'runtime environment' but the `OptimizerContext` only provides access to `ConfigOptions` and `Extensions`. The runtime environment is not actually included in the context. Consider updating the documentation to accurately reflect what is provided: 'session configuration and extensions' without mentioning runtime environment. ```suggestion /// provides access to the full optimizer context, including session configuration /// and extensions. ``` ########## datafusion/physical-optimizer/src/optimizer.rs: ########## @@ -38,9 +38,49 @@ use crate::update_aggr_exprs::OptimizeAggregateOrder; use crate::limit_pushdown_past_window::LimitPushPastWindows; use datafusion_common::config::ConfigOptions; -use datafusion_common::Result; +use datafusion_common::{internal_err, Result}; +use datafusion_execution::config::{Extensions, SessionConfig}; use datafusion_physical_plan::ExecutionPlan; +/// Context for optimizing physical plans. +/// +/// This context provides access to session configuration and optimizer extensions. +/// +/// Similar to [`TaskContext`] which provides context during execution, +/// `OptimizerContext` provides context during optimization. +/// +/// [`TaskContext`]: https://docs.rs/datafusion/latest/datafusion/execution/struct.TaskContext.html +#[derive(Debug, Clone)] +pub struct OptimizerContext { + /// Config options + config_options: Arc<ConfigOptions>, + /// Extensions + extensions: Arc<Extensions>, +} + +impl OptimizerContext { + /// Create a new OptimizerContext + pub fn new_from_session_config(session_config: &SessionConfig) -> Self { + Self { + config_options: Arc::clone(session_config.options()), + extensions: Arc::clone(session_config.extensions()), + } + } + + /// Return a reference to the configuration options + /// + /// This is a convenience method that returns the [`ConfigOptions`] + /// from the [`SessionConfig`]. + pub fn config_options(&self) -> &ConfigOptions { + &self.config_options + } + + /// Return a reference to the extensions + pub fn extensions(&self) -> &Extensions { + &self.extensions + } +} + /// `PhysicalOptimizerRule` transforms one ['ExecutionPlan'] into another which Review Comment: Inconsistent bracket style in documentation reference. Should use backticks with square brackets: `['ExecutionPlan']` should be changed to `[`ExecutionPlan`]` for proper rustdoc linking syntax. ```suggestion /// `PhysicalOptimizerRule` transforms one [`ExecutionPlan`] into another which ``` -- 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]
