-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Move optimizer init to optimizer crate #3692
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks wonderful to me -- thank you @andygrove
/// Create a new optimizer with the given rules | ||
pub fn new(rules: Vec<Arc<dyn OptimizerRule + Send + Sync>>) -> Self { | ||
pub fn with_rules(rules: Vec<Arc<dyn OptimizerRule + Send + Sync>>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that is odd now is that we pass a config into the constructor and also pass it into the optimize method - I plan to create a separate PR to see if we can change that.
let mut config = OptimizerConfig::new().with_skip_failing_rules(false);
let optimizer = Optimizer::new(&config);
optimizer.optimize(&plan, &mut config, &observe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... we need to figure out what to do with this id generator first
Benchmark runs are scheduled for baseline = 8415368 and contender = 6b282ec. 6b282ec is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3524
Rationale for this change
We need the optimizer tests in the optimizer crate to run against the same set of rules that DataFusion actually uses.
What changes are included in this PR?
Move the list of rules to the optimizer crate.
Are there any user-facing changes?