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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Sep 20, 2024

Which issue does this PR close?

Part of #12555

Rationale for this change

I would like DataFusion to be easiser to configure and use, and having Debug impls for
common structs would be a big help.

What changes are included in this PR?

  1. Require Debug for AnalyzerRule, FunctionRewriter, and OptimizerRule
  2. #derive this trait as necessary

Are these changes tested?

Yes buy CI and the compiler

Are there any user-facing changes?

Technically this is an API change as it also adds the requirement for Debug for various optimizer rules, but I think that is good thing.

@github-actions github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate labels Sep 20, 2024
@alamb alamb marked this pull request as ready for review September 20, 2024 13:07
@@ -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

@@ -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

@alamb alamb added the api change Changes the API exposed to users of the crate label Sep 20, 2024
Copy link
Contributor

@goldmedal goldmedal left a comment

Choose a reason for hiding this comment

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

Thanks @alamb It's a nice change for the user who wants to implement the custom rules 👍

Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change Changes the API exposed to users of the crate core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants