Implement AsRef for Expr
#17819
Implement AsRef for Expr
#17819
Conversation
This allows writing a function that accepts `&[Expr]` or `&[&Expr]`, thus allowing less cloning when inspecting expression trees.
Clone lazily, only when needed
github-actions
bot
added
logical-expr
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
This just feels wrong to me because if Rust meant to implict allow converting a type to a reference the would have built it into the language
I googled around , however, and didn't find many compelling reasons, and even found a few where it looked like maybe it would be automatically implemented but couldn't for some other reasons
- https://users.rust-lang.org/t/whats-the-reason-of-implementing-asref-t-for-t/120931/6
- https://users.rust-lang.org/t/why-not-impl-asref-t-for-t/77877
- https://users.rust-lang.org/t/when-to-use-asref-t-vs-t/9312
However, given this PR avoids some clones and I don't think there is anything wrong , I think it could be merged as is
I have an alternate suggestion which avoids the clone() as well here:
But realistically if I am honest that is a matter of preference
Thank you @findepi
| // If the filters can be resolved using only partition cols, there is no need to | ||
| // pushdown it to TableScan, otherwise, `unhandled` pruning predicates will be generated | ||
| let (partition_filters, filters): (Vec<_>, Vec<_>) = | ||
| // TODO avoid .cloned() here by accepting &[&Expr] or equivalent downstream |
There was a problem hiding this comment.
did you mean to do this in this PR?
There was a problem hiding this comment.
no, that is fine, I was just confused
| // Split the parent predicate into individual conjunctive parts. | ||
| let predicates = parent_predicate | ||
| .map_or_else(Vec::new, |pred| split_conjunction_owned(pred.clone())); | ||
| let predicates = |
There was a problem hiding this comment.
this certainly looks nicer but it seems like the expressions are still cloned at the end. Though maybe in this case only the conjunctions are cloned (not the BinaryExprs with AND) so it would be an improvement
There was a problem hiding this comment.
this certainly looks nicer but it seems like the expressions are still cloned at the end.
Correct. The intention was to delay cloning, but I admit the Avoid clones in push_down_join commit is not ideal. I realized that we avoid cloning only in case there wasn't much to clone in the first place. I will revert the commit.
What I wanted to show with this commit is that implementing AsRef<Expr> for Expr allows writing APIs that are impossible to model otherwise. Note also that downstream projects have no way to add such impl or workaround lack of it.
This works fine today, but the IntoIterator-based API is significantly less powerful. |
This reverts commit d886be5. Revert per PR discussion. The change didn't buy much.
Yeah, I am fine with this change (though note #17831 the additional flexibility of passing in a slice is not actually used) |
|
merging per #17831 (comment) |
|
Thanks @findepi |
2 participants
This allows writing a function that accepts
&[Expr]or&[&Expr], thus allowing less cloning when inspecting expression trees.