gabotechs commented on code in PR #23842: URL: https://github.com/apache/datafusion/pull/23842#discussion_r3663628403
########## datafusion/session/src/physical_optimizer.rs: ########## @@ -0,0 +1,73 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Physical optimizer interfaces. + +use std::fmt::Debug; +use std::sync::Arc; + +use datafusion_common::Result; +use datafusion_common::config::ConfigOptions; +use datafusion_physical_plan::ExecutionPlan; +use datafusion_physical_plan::operator_statistics::StatisticsRegistry; + +/// Context available to physical optimizer rules. +/// +/// This trait provides access to configuration options and an optional statistics +/// registry for enhanced statistics lookup. +pub trait PhysicalOptimizerContext: Send + Sync { + /// Returns the configuration options. + fn config_options(&self) -> &ConfigOptions; + + /// Returns the statistics registry for enhanced statistics lookup. + /// + /// Returns `None` if no registry is configured, in which case rules + /// should fall back to using [`ExecutionPlan::partition_statistics`]. + fn statistics_registry(&self) -> Option<&StatisticsRegistry> { + None + } +} + +/// Transforms one [`ExecutionPlan`] into another that computes the same results, +/// but may do so more efficiently. +pub trait PhysicalOptimizerRule: Debug + std::any::Any { Review Comment: Seems like we've lost some documentation while moving this here? I'd say the docs in the previous struct are still relevant no? For example: ```rust /// `PhysicalOptimizerRule` transforms one ['ExecutionPlan'] into another which /// computes the same results, but in a potentially more efficient way. /// /// Use [`SessionState::add_physical_optimizer_rule`] to register additional /// `PhysicalOptimizerRule`s. /// /// [`SessionState::add_physical_optimizer_rule`]: https://docs.rs/datafusion/latest/datafusion/execution/session_state/struct.SessionState.html#method.add_physical_optimizer_rule ``` ########## datafusion/session/src/session.rs: ########## @@ -89,6 +92,30 @@ pub trait Session: Send + Sync { self.config().options() } + /// Return the query planner for this session. + fn query_planner(&self) -> Arc<dyn QueryPlanner + Send + Sync>; + + /// Optimize a logical plan. + /// + /// The default implementation returns the plan unchanged. Sessions that use + /// the default query planning implementation should override this method. + fn optimize(&self, plan: &LogicalPlan) -> Result<LogicalPlan> { + Ok(plan.clone()) + } + + /// Return the physical optimizer rules for this session. + /// + /// The default implementation returns no rules. Sessions that use the + /// default physical planner should override this method. + fn physical_optimizers(&self) -> &[Arc<dyn PhysicalOptimizerRule + Send + Sync>] { + &[] Review Comment: :thinking: I wonder if this should be defaulting to the default physical optimizer rules. I'm not sure if DataFusion is even useful without those: https://github.com/apache/datafusion/blob/main/datafusion/physical-optimizer/src/optimizer.rs#L91-L97 ########## datafusion/core/src/physical_planner.rs: ########## @@ -57,6 +55,7 @@ use crate::physical_plan::{ displayable, windows, }; use crate::schema_equivalence::schema_satisfied_by; +use datafusion_session::{PhysicalOptimizerContext, Session}; use arrow::array::{RecordBatch, builder::StringBuilder}; use arrow::compute::SortOptions; Review Comment: nit: just some import ordering hygiene: ```suggestion use datafusion_session::{PhysicalOptimizerContext, Session}; use arrow::array::{RecordBatch, builder::StringBuilder}; ``` ########## datafusion/session/src/session.rs: ########## @@ -89,6 +92,30 @@ pub trait Session: Send + Sync { self.config().options() } + /// Return the query planner for this session. + fn query_planner(&self) -> Arc<dyn QueryPlanner + Send + Sync>; + + /// Optimize a logical plan. + /// + /// The default implementation returns the plan unchanged. Sessions that use + /// the default query planning implementation should override this method. + fn optimize(&self, plan: &LogicalPlan) -> Result<LogicalPlan> { + Ok(plan.clone()) + } + + /// Return the physical optimizer rules for this session. + /// + /// The default implementation returns no rules. Sessions that use the + /// default physical planner should override this method. + fn physical_optimizers(&self) -> &[Arc<dyn PhysicalOptimizerRule + Send + Sync>] { + &[] + } + + /// Return the optional statistics registry used during physical optimization. + fn statistics_registry(&self) -> Option<&StatisticsRegistry> { + None + } Review Comment: Is there anything that we should be doing for the other methods from `SessionState`? (expr_planners, relation_planners, table_factories, serializer_registry or function_factory). I imagine that if any `QueryPlanner` or `ExtensionPlanner` was relying on those, they will no longer be able to use them without downcasting. ########## datafusion/core/src/physical_planner.rs: ########## @@ -120,162 +119,31 @@ use itertools::{Itertools, multiunzip}; use log::debug; use tokio::sync::Mutex; -/// Physical query planner that converts a `LogicalPlan` to an -/// `ExecutionPlan` suitable for execution. -#[async_trait] -pub trait PhysicalPlanner: Send + Sync { - /// Create a physical plan from a logical plan - async fn create_physical_plan( - &self, - logical_plan: &LogicalPlan, - session_state: &SessionState, - ) -> Result<Arc<dyn ExecutionPlan>>; +// Re-export from this module for backwards compatibility. +pub use datafusion_session::{ExtensionPlanner, PhysicalPlanner}; - /// Create a physical expression from a logical expression - /// suitable for evaluation - /// - /// `expr`: the expression to convert - /// - /// `input_dfschema`: the logical plan schema for evaluating `expr` - /// - /// `planning_ctx`: the [`PhysicalPlanningContext`] used to resolve - /// `Expr::ScalarSubquery` nodes. During physical planning the planner - /// threads the context of the plan currently being converted to a physical - /// plan (for example into [`ExtensionPlanner::plan_extension`], which - /// should forward it here). Callers creating physical expressions outside - /// of a plan should pass `&PhysicalPlanningContext::default()`. - fn create_physical_expr( - &self, - expr: &Expr, - input_dfschema: &DFSchema, - session_state: &SessionState, - planning_ctx: &PhysicalPlanningContext, - ) -> Result<Arc<dyn PhysicalExpr>>; +struct SessionOptimizerContext<'a> { + session: &'a dyn Session, } Review Comment: It's a shame that we cannot implement `PhysicalOptimizerContext` automatically for any `dyn Session` ########## datafusion/session/src/physical_optimizer.rs: ########## @@ -0,0 +1,73 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Physical optimizer interfaces. + +use std::fmt::Debug; +use std::sync::Arc; + +use datafusion_common::Result; +use datafusion_common::config::ConfigOptions; +use datafusion_physical_plan::ExecutionPlan; +use datafusion_physical_plan::operator_statistics::StatisticsRegistry; + +/// Context available to physical optimizer rules. +/// +/// This trait provides access to configuration options and an optional statistics +/// registry for enhanced statistics lookup. +pub trait PhysicalOptimizerContext: Send + Sync { + /// Returns the configuration options. + fn config_options(&self) -> &ConfigOptions; + + /// Returns the statistics registry for enhanced statistics lookup. + /// + /// Returns `None` if no registry is configured, in which case rules + /// should fall back to using [`ExecutionPlan::partition_statistics`]. + fn statistics_registry(&self) -> Option<&StatisticsRegistry> { + None + } +} Review Comment: :thinking: these two methods also exist on `Session`. I wonder if that can introduce some ambiguity that the compiler surfaces as "multiple applicable items in scope". An alternative that comes to mind is to rely on trait composition: instead of duplicating methods in `config_options` and `statistics_registry`. This probably applies to more traits rather than just this. Trait upcasting was stabilized a while ago in the Rust compiler. Just throwing the idea, don't have a strong opinion here. -- 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]
