-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 remove Expr
clones
#10115
Refactor UnwrapCastInComparison
to remove Expr
clones
#10115
Conversation
cefe8b9
to
295ef58
Compare
295ef58
to
22b55f4
Compare
The test failure:
seems unrelated. |
fn f_up(&mut self, expr: Expr) -> Result<Transformed<Expr>> { | ||
match &expr { | ||
fn f_up(&mut self, mut expr: Expr) -> Result<Transformed<Expr>> { | ||
match &mut expr { |
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.
I was trying a few different patterns to remove Expr
clones in this rule. The match &mut expr
way seems to be most straightforward as expr
is still available after pattern matching so we can return it unchanged if needed (return Ok(Transformed::no(expr));
) but we can also mutate some parts of the expr
if needed.
}; | ||
// unwrap the cast/try_cast for the left expr | ||
**left = | ||
mem::replace(left_expr, Expr::Literal(ScalarValue::Null)); |
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.
Would be nice to have a default Expr
so that we could use mem::take()
.
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.
Oh yeah!
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.
Shall I open a follow-up PR that adds default for Expr
?
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.
mem::take
is a nice thing to avoid cloning, I think I used it to avoid String cloning other day, and we can have a default Expr with panicking inside I'd say...
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.
I've opened a minor PR to add default for Expr
: #10127, but as Expr
is just an enum, I don't know how to panic inside it.
CI failure I think is due to #10119, so I retriggered the run |
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.
Very nice @peter-toth -- I actually tried this msyself last week and I was not able to get it to work. 👌
// do nothing | ||
} | ||
Expr::BinaryExpr(BinaryExpr { left, op, right }) | ||
if { |
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.
Today I learned you can add statements in a {}
as part of a match clause. 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.
+1. I used to wrap another function.
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 @alamb @comphead |
Thanks all for the review! |
Which issue does this PR close?
Part of #9637, follow-up to #10087.
Rationale for this change
Eliminates remaining
Expr
clones.What changes are included in this PR?
This PR:
UnwrapCastExprRewriter
to removeExpr
clones.BinaryExpr
case as if any error occurs this optimizer rule should return the original expression.Are these changes tested?
Yes, with existing UTs.
Are there any user-facing changes?
No.