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

Minor: Add getter for logical optimizer rules #12379

Merged
merged 3 commits into from
Sep 8, 2024

Conversation

maronavenue
Copy link
Contributor

Which issue does this PR close?

N/A

Rationale for this change

My team and I have been exploring Datafusion's potential to federate our custom sources performantly. Part of that is tuning optimizers depending on our needs. Noticed that there isn't a convenience method in SessionState yet that returns logical optimizer rules compared to its physical counterpart. Would be great for our POC work to quickly inspect them outside (and/or alongside) debugger or observer.

What changes are included in this PR?

Adds a getter method optimizers() in SessionState in parity to the physical_optimizers() counterpart.

Are these changes tested?

Yes, ran new and existing tests. Also manually tested from our existing POC code.

Are there any user-facing changes?

Examples:

let state = SessionStateBuilder::new()
    .with_optimizer_rules(vec![
        Arc::new(UnwrapCastInComparison::new()),
        Arc::new(SimplifyExpressions::new()),
        Arc::new(OptimizeProjections::new()),
        Arc::new(PushDownFilter::new()),
    ])
    .build();

let optimizer_rules = state.optimizers();
for rule in optimizer_rules {
    println!("Optimizer: {}", rule.name());
}

@github-actions github-actions bot added the core Core DataFusion crate label Sep 7, 2024
@maronavenue
Copy link
Contributor Author

This is my first time contributing. Please let me know if I missed any. Tagging for CI, @alamb @andygrove

Copy link
Member

@Weijun-H Weijun-H left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @maronavenue


#[test]
fn test_session_state_with_optimizer_rules() {
// test building sessions with supplied rules
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
// test building sessions with supplied rules
struct DummyRule {}
impl OptimizerRule for DummyRule {
fn name(&self) -> &str {
"dummy_rule"
}
}
// test building sessions with supplied rules

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Weijun-H - Thanks for having a look! Applied your feedback.

bba3fe5

Comment on lines 1952 to 1959

struct DummyRule {}

impl OptimizerRule for DummyRule {
fn name(&self) -> &str {
"dummy_rule"
}
}
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
struct DummyRule {}
impl OptimizerRule for DummyRule {
fn name(&self) -> &str {
"dummy_rule"
}
}

@Weijun-H Weijun-H merged commit 3e1850d into apache:main Sep 8, 2024
24 checks passed
@Weijun-H
Copy link
Member

Weijun-H commented Sep 8, 2024

Thanks @maronavenue for contribution and @Dandandan for review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants