Remove deprecated function OptimizerRule::try_optimize
#15051
+16
−88
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Remove intermediate `optimize_plan_node` function and directly call `rule.rewrite()` method
ctsk
reviewed
Mar 6, 2025
Comment on lines
+289
to
+293
| { | ||
| let rule: &dyn OptimizerRule = self.rule; | ||
| let config: &dyn OptimizerConfig = self.config; | ||
| rule.rewrite(node, config) | ||
| } |
There was a problem hiding this comment.
I'd just write
self.rule.rewrite(node, self.config):)
| @@ -386,7 +354,10 @@ impl Optimizer { | |||
| &mut Rewriter::new(apply_order, rule.as_ref(), config), | |||
| ), | |||
| // rule handles recursion itself | |||
| None => optimize_plan_node(new_plan, rule.as_ref(), config), | |||
| None => { | |||
| let rule: &dyn OptimizerRule = rule.as_ref(); | |||
alamb
changed the title
optimizeroptimizer: remove OptimizerRule::try_optimize
alamb
approved these changes
Mar 6, 2025
There was a problem hiding this comment.
Thank you @qazxcdswe123 and @ctsk for the review
I think @ctsk 's comments and suggestions are worth considering and ideally doing before we merge this PR but I don't think they are strictly necessary and we could do them as follow on PRs as well
| @@ -107,7 +89,7 @@ pub trait OptimizerRule: Debug { | |||
| /// if the plan was rewritten and `Transformed::no` if it was not. | |||
| /// | |||
| /// Note: this function is only called if [`Self::supports_rewrite`] returns | |||
There was a problem hiding this comment.
I think after this PR supports_rewrite becomes a noop (doesn't do anything) so I recommend we also mark it as deprecated as well (deprecated as of 47.0.0)
| @@ -62,8 +62,7 @@ impl OptimizerRule for SimplifyExpressions { | |||
| true | |||
| } | |||
|
|
|||
| /// if supports_owned returns true, the Optimizer calls | |||
| /// [`Self::rewrite`] instead of [`Self::try_optimize`] | |||
| /// if supports_owned returns true, the Optimizer calls [`Self::rewrite`] | |||
There was a problem hiding this comment.
I don't understand what this comments means anymore -- I think we can simply delete it
alamb
changed the title
optimizer: remove OptimizerRule::try_optimizeOptimizerRule::try_optimize
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up for #15027
optimizeroptimize_plan_nodeI've also noticed that, all
supports_rewritereturn true and not used anywhere, should we also remove it?