Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require Debug for AnalyzerRule, FunctionRewriter, and OptimizerRule #12556

Merged
merged 3 commits into from
Sep 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions datafusion-examples/examples/optimizer_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ pub async fn main() -> Result<()> {

/// An example OptimizerRule that replaces all `col = <const>` predicates with a
/// user defined function
#[derive(Default, Debug)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the end user visible change -- basically you have to #[derive(Debug)] if you haven't already

struct MyOptimizerRule {}

impl OptimizerRule for MyOptimizerRule {
Expand Down
1 change: 1 addition & 0 deletions datafusion/core/src/execution/session_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1905,6 +1905,7 @@ mod tests {

#[test]
fn test_session_state_with_optimizer_rules() {
#[derive(Default, Debug)]
struct DummyRule {}

impl OptimizerRule for DummyRule {
Expand Down
3 changes: 3 additions & 0 deletions datafusion/core/tests/user_defined/user_defined_plan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,9 @@ impl QueryPlanner for TopKQueryPlanner {
}
}

#[derive(Default, Debug)]
struct TopKOptimizerRule {}

impl OptimizerRule for TopKOptimizerRule {
fn name(&self) -> &str {
"topk"
Expand Down Expand Up @@ -686,6 +688,7 @@ impl RecordBatchStream for TopKReader {
}
}

#[derive(Default, Debug)]
struct MyAnalyzerRule {}

impl AnalyzerRule for MyAnalyzerRule {
Expand Down
3 changes: 2 additions & 1 deletion datafusion/expr/src/expr_rewriter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

use std::collections::HashMap;
use std::collections::HashSet;
use std::fmt::Debug;
use std::sync::Arc;

use crate::expr::{Alias, Sort, Unnest};
Expand All @@ -42,7 +43,7 @@ pub use order_by::rewrite_sort_cols_by_aggs;
/// `Operator::ArrowAt`, but can be implemented by calling a function
/// `array_concat` from the `functions-nested` crate.
// This is not used in datafusion internally, but it is still helpful for downstream project so don't remove it.
pub trait FunctionRewrite {
pub trait FunctionRewrite: Debug {
/// Return a human readable name for this rewrite
fn name(&self) -> &str;

Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/analyzer/count_wildcard_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use datafusion_expr::{lit, Expr, LogicalPlan, WindowFunctionDefinition};
/// Rewrite `Count(Expr:Wildcard)` to `Count(Expr:Literal)`.
///
/// Resolves issue: <https://github.com/apache/datafusion/issues/5473>
#[derive(Default)]
#[derive(Default, Debug)]
pub struct CountWildcardRule {}

impl CountWildcardRule {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/analyzer/expand_wildcard_rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use datafusion_expr::utils::{
};
use datafusion_expr::{Expr, LogicalPlan, Projection, SubqueryAlias};

#[derive(Default)]
#[derive(Default, Debug)]
pub struct ExpandWildcardRule {}

impl ExpandWildcardRule {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/analyzer/function_rewrite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use datafusion_expr::LogicalPlan;
use std::sync::Arc;

/// Analyzer rule that invokes [`FunctionRewrite`]s on expressions
#[derive(Default)]
#[derive(Default, Debug)]
pub struct ApplyFunctionRewrites {
/// Expr --> Function writes to apply
function_rewrites: Vec<Arc<dyn FunctionRewrite + Send + Sync>>,
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/analyzer/inline_table_scan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use datafusion_expr::{logical_plan::LogicalPlan, Expr, LogicalPlanBuilder};

/// Analyzed rule that inlines TableScan that provide a [`LogicalPlan`]
/// (DataFrame / ViewTable)
#[derive(Default)]
#[derive(Default, Debug)]
pub struct InlineTableScan;

impl InlineTableScan {
Expand Down
6 changes: 4 additions & 2 deletions datafusion/optimizer/src/analyzer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
// under the License.

//! [`Analyzer`] and [`AnalyzerRule`]

use std::fmt::Debug;
use std::sync::Arc;

use log::debug;
Expand Down Expand Up @@ -60,7 +62,7 @@ pub mod type_coercion;
/// `AnalyzerRule`s.
///
/// [`SessionState::add_analyzer_rule`]: https://docs.rs/datafusion/latest/datafusion/execution/session_state/struct.SessionState.html#method.add_analyzer_rule
pub trait AnalyzerRule {
pub trait AnalyzerRule: Debug {
/// Rewrite `plan`
fn analyze(&self, plan: LogicalPlan, config: &ConfigOptions) -> Result<LogicalPlan>;

Expand All @@ -72,7 +74,7 @@ pub trait AnalyzerRule {
///
/// An `Analyzer` transforms a `LogicalPlan`
/// prior to the rest of the DataFusion optimization process.
#[derive(Clone)]
#[derive(Clone, Debug)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be nice to be able to see what rules are registered with the analyzer/optimizer

pub struct Analyzer {
/// Expr --> Function writes to apply prior to analysis passes
pub function_rewrites: Vec<Arc<dyn FunctionRewrite + Send + Sync>>,
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/analyzer/type_coercion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ use datafusion_expr::{

/// Performs type coercion by determining the schema
/// and performing the expression rewrites.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct TypeCoercion {}

impl TypeCoercion {
Expand Down
1 change: 1 addition & 0 deletions datafusion/optimizer/src/common_subexpr_eliminate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ type CommonExprs<'n> = IndexMap<Identifier<'n>, (Expr, String)>;
/// ProjectionExec(exprs=[extract (day from new_col), extract (year from new_col)]) <-- reuse here
/// ProjectionExec(exprs=[to_date(c1) as new_col]) <-- compute to_date once
/// ```
#[derive(Debug)]
pub struct CommonSubexprEliminate {
random_state: RandomState,
}
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/decorrelate_predicate_subquery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use datafusion_expr::{
use log::debug;

/// Optimizer rule for rewriting predicate(IN/EXISTS) subquery to left semi/anti joins
#[derive(Default)]
#[derive(Default, Debug)]
pub struct DecorrelatePredicateSubquery {}

impl DecorrelatePredicateSubquery {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_cross_join.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use datafusion_expr::logical_plan::{
use datafusion_expr::utils::{can_hash, find_valid_equijoin_key_pair};
use datafusion_expr::{build_join_schema, ExprSchemable, Operator};

#[derive(Default)]
#[derive(Default, Debug)]
pub struct EliminateCrossJoin;

impl EliminateCrossJoin {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_duplicated_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use datafusion_expr::{Aggregate, Expr, Sort, SortExpr};
use indexmap::IndexSet;
use std::hash::{Hash, Hasher};
/// Optimization rule that eliminate duplicated expr.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct EliminateDuplicatedExpr;

impl EliminateDuplicatedExpr {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use crate::{OptimizerConfig, OptimizerRule};
///
/// This saves time in planning and executing the query.
/// Note that this rule should be applied after simplify expressions optimizer rule.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct EliminateFilter;

impl EliminateFilter {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_group_by_constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use datafusion_expr::{Aggregate, Expr, LogicalPlan, LogicalPlanBuilder, Volatili
/// Optimizer rule that removes constant expressions from `GROUP BY` clause
/// and places additional projection on top of aggregation, to preserve
/// original schema
#[derive(Default)]
#[derive(Default, Debug)]
pub struct EliminateGroupByConstant {}

impl EliminateGroupByConstant {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_join.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use datafusion_expr::{

/// Eliminates joins when join condition is false.
/// Replaces joins when inner join condition is true with a cross join.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct EliminateJoin;

impl EliminateJoin {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_limit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use std::sync::Arc;
/// plan with an empty relation.
///
/// This rule also removes OFFSET 0 from the [LogicalPlan]
#[derive(Default)]
#[derive(Default, Debug)]
pub struct EliminateLimit;

impl EliminateLimit {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_nested_union.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use datafusion_expr::{Distinct, LogicalPlan, Union};
use itertools::Itertools;
use std::sync::Arc;

#[derive(Default)]
#[derive(Default, Debug)]
/// An optimization rule that replaces nested unions with a single union.
pub struct EliminateNestedUnion;

Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_one_union.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use std::sync::Arc;

use crate::optimizer::ApplyOrder;

#[derive(Default)]
#[derive(Default, Debug)]
/// An optimization rule that eliminates union with one element.
pub struct EliminateOneUnion;

Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/eliminate_outer_join.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use datafusion_common::tree_node::Transformed;
use datafusion_expr::expr::{BinaryExpr, Cast, TryCast};
use std::sync::Arc;

#[derive(Default)]
///
/// Attempt to replace outer joins with inner joins.
///
Expand All @@ -49,6 +48,7 @@ use std::sync::Arc;
/// filters from the WHERE clause return false while any inputs are
/// null and columns of those quals are come from nullable side of
/// outer join.
#[derive(Default, Debug)]
pub struct EliminateOuterJoin;

impl EliminateOuterJoin {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/extract_equijoin_predicate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ type EquijoinPredicate = (Expr, Expr);
/// has one equijoin predicate (`A.x = B.y`) and one filter predicate (`B.z > 50`).
/// See [find_valid_equijoin_key_pair] for more information on what predicates
/// are considered equijoins.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct ExtractEquijoinPredicate;

impl ExtractEquijoinPredicate {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/filter_null_join_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use std::sync::Arc;
/// The FilterNullJoinKeys rule will identify joins with equi-join conditions
/// where the join key is nullable and then insert an `IsNotNull` filter on the nullable side since null values
/// can never match.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct FilterNullJoinKeys {}

impl OptimizerRule for FilterNullJoinKeys {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/optimize_projections/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ use datafusion_common::tree_node::{
/// The rule analyzes the input logical plan, determines the necessary column
/// indices, and then removes any unnecessary columns. It also removes any
/// unnecessary projections from the plan tree.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct OptimizeProjections {}

impl OptimizeProjections {
Expand Down
8 changes: 6 additions & 2 deletions datafusion/optimizer/src/optimizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
//! [`Optimizer`] and [`OptimizerRule`]

use std::collections::HashSet;
use std::fmt::Debug;
use std::sync::Arc;

use chrono::{DateTime, Utc};
Expand Down Expand Up @@ -70,7 +71,7 @@ use crate::utils::log_plan;
/// [`AnalyzerRule`]: crate::analyzer::AnalyzerRule
/// [`SessionState::add_optimizer_rule`]: https://docs.rs/datafusion/latest/datafusion/execution/session_state/struct.SessionState.html#method.add_optimizer_rule

pub trait OptimizerRule {
pub trait OptimizerRule: Debug {
/// Try and rewrite `plan` to an optimized form, returning None if the plan
/// cannot be optimized by this rule.
///
Expand Down Expand Up @@ -214,7 +215,7 @@ impl OptimizerConfig for OptimizerContext {
}

/// A rule-based optimizer.
#[derive(Clone)]
#[derive(Clone, Debug)]
pub struct Optimizer {
/// All optimizer rules to apply
pub rules: Vec<Arc<dyn OptimizerRule + Send + Sync>>,
Expand Down Expand Up @@ -666,6 +667,7 @@ mod tests {

fn observe(_plan: &LogicalPlan, _rule: &dyn OptimizerRule) {}

#[derive(Default, Debug)]
struct BadRule {}

impl OptimizerRule for BadRule {
Expand All @@ -687,6 +689,7 @@ mod tests {
}

/// Replaces whatever plan with a single table scan
#[derive(Default, Debug)]
struct GetTableScanRule {}

impl OptimizerRule for GetTableScanRule {
Expand All @@ -713,6 +716,7 @@ mod tests {
/// A goofy rule doing rotation of columns in all projections.
///
/// Useful to test cycle detection.
#[derive(Default, Debug)]
struct RotateProjectionRule {
// reverse exprs instead of rotating on the first pass
reverse_on_first_pass: Mutex<bool>,
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/propagate_empty_relation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use crate::optimizer::ApplyOrder;
use crate::{OptimizerConfig, OptimizerRule};

/// Optimization rule that bottom-up to eliminate plan by propagating empty_relation.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct PropagateEmptyRelation;

impl PropagateEmptyRelation {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/push_down_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ use crate::{OptimizerConfig, OptimizerRule};
/// reaches a plan node that does not commute with that filter, it adds the
/// filter to that place. When it passes through a projection, it re-writes the
/// filter's expression taking into account that projection.
#[derive(Default)]
#[derive(Default, Debug)]
pub struct PushDownFilter {}

/// For a given JOIN type, determine whether each input of the join is preserved
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/push_down_limit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use datafusion_expr::logical_plan::{Join, JoinType, Limit, LogicalPlan};
/// Optimization rule that tries to push down `LIMIT`.
///
//. It will push down through projection, limits (taking the smaller limit)
#[derive(Default)]
#[derive(Default, Debug)]
pub struct PushDownLimit {}

impl PushDownLimit {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/replace_distinct_aggregate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ use datafusion_expr::{Aggregate, Distinct, DistinctOn, Expr, LogicalPlan};
/// ```

/// Optimizer that replaces logical [[Distinct]] with a logical [[Aggregate]]
#[derive(Default)]
#[derive(Default, Debug)]
pub struct ReplaceDistinctWithAggregate {}

impl ReplaceDistinctWithAggregate {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/rewrite_disjunctive_predicate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ use datafusion_expr::{Expr, LogicalPlan, Operator};
/// )
/// ```
///
#[derive(Default)]
#[derive(Default, Debug)]
pub struct RewriteDisjunctivePredicate;

impl RewriteDisjunctivePredicate {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/scalar_subquery_to_join.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ use datafusion_expr::utils::conjunction;
use datafusion_expr::{expr, EmptyRelation, Expr, LogicalPlan, LogicalPlanBuilder};

/// Optimizer rule for rewriting subquery filters to joins
#[derive(Default)]
#[derive(Default, Debug)]
pub struct ScalarSubqueryToJoin {}

impl ScalarSubqueryToJoin {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ use super::ExprSimplifier;
/// `Filter: b > 2`
///
/// [`Expr`]: datafusion_expr::Expr
#[derive(Default)]
#[derive(Default, Debug)]
pub struct SimplifyExpressions {}

impl OptimizerRule for SimplifyExpressions {
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/single_distinct_to_groupby.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ use hashbrown::HashSet;
/// )
/// GROUP BY a
/// ```
#[derive(Default)]
#[derive(Default, Debug)]
pub struct SingleDistinctToGroupBy {}

const SINGLE_DISTINCT_ALIAS: &str = "alias1";
Expand Down
2 changes: 1 addition & 1 deletion datafusion/optimizer/src/unwrap_cast_in_comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ use datafusion_expr::{lit, Expr, ExprSchemable, LogicalPlan};
/// Filter: c1 > INT32(10)
/// ```
///
#[derive(Default)]
#[derive(Default, Debug)]
pub struct UnwrapCastInComparison {}

impl UnwrapCastInComparison {
Expand Down