Skip to content

Minor: Add getter for logical optimizer rules - #12379

Merged
Weijun-H merged 3 commits into
apache:mainfrom
maronavenue:chore/get-optimizer-rules
Sep 8, 2024
Merged

Weijun-H merged 3 commits into
apache:mainfrom
maronavenue:chore/get-optimizer-rules

Conversation

@maronavenue

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

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

@Weijun-H Weijun-H left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@Weijun-H

Weijun-H commented Sep 8, 2024

Copy link
Copy Markdown
Member

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