-
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
Refactor UnwrapCastInComparison
to implement OptimizerRule::rewrite()
#10087
Refactor UnwrapCastInComparison
to implement OptimizerRule::rewrite()
#10087
Conversation
UnwrapCastInComparison
to use rewrite()
UnwrapCastInComparison
to implement OptimizerRule::rewrite()
d6e0c8f
to
684fa0a
Compare
684fa0a
to
6c4256a
Compare
6c4256a
to
57605af
Compare
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.
Thank you @peter-toth -- amazing how simple these PRs are now that we have the pattern down.- There are still some Expr
copies left in the actual implementation I noticed, but this gets a bunch of the structural copying out of the way
Very nice
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.
👍
Thanks @peter-toth |
I will try to eliminate those in a follow-up PR. Thanks for the review @alamb, @jayzhan211! |
@alamb, @jayzhan211 here is a follow-up PR to remove remaining |
Which issue does this PR close?
Part of #9637.
Rationale for this change
Eliminates a
LogicalPlan::with_new_exprs()
call and soExpr
cloning.What changes are included in this PR?
This PR refactors
UnwrapCastInComparison::try_optimize()
toUnwrapCastInComparison::rewrite()
.Are these changes tested?
Yes, with existing UTs.
Are there any user-facing changes?
No.