refactor: reduce allocations in push down filter#10567
Conversation
erratic-pattern
force-pushed
the
adam/reduce-alloc-push-down-filter
branch
from
d9a4d7f to
17b99b9
Compare
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
thanks @erratic-pattern
please help me to understand how the approach in the PR differs from what we have now. Its replaces clone on Arcs to Arc::clone, which imho the same https://doc.rust-lang.org/std/sync/struct.Arc.html#cloning-references ?
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
please help me to understand how the approach in the PR differs from what we have now. Its replaces clone on Arcs to Arc::clone, which imho the same https://doc.rust-lang.org/std/sync/struct.Arc.html#cloning-references ?
I agree with @comphead calling Arc::clone is the same thing, though it is more explicit that only an Arc is being copied so I think it is an improvement in the code
I think I found a few other Expr::clone that this PR avoids, so from that perspective I think it is an improvement.
Thank you @comphead and @erratic-pattern
| let replaced_push_predicates = push_predicates | ||
| .iter() | ||
| .map(|expr| replace_cols_by_name(expr.clone(), &replace_map)) | ||
| .into_iter() |
There was a problem hiding this comment.
this looks like one example of less cloning
| LogicalPlan::Join(join) => push_down_join(join, Some(&filter.predicate)), | ||
| LogicalPlan::CrossJoin(cross_join) => { | ||
| let predicates = split_conjunction_owned(filter.predicate.clone()); | ||
| let predicates = split_conjunction_owned(filter.predicate); |
There was a problem hiding this comment.
likewise here we avoid an additional clone
| .map(|(pred, _)| pred.clone()) | ||
| .collect(); | ||
|
|
||
| let new_scan = LogicalPlan::TableScan(TableScan { |
There was a problem hiding this comment.
👍 this also looks like it might save some copying
|
Thanks again @erratic-pattern |
|
Yes the |
3 participants
Not sure which issue to link here, but here's some changes to reduce allocations in
PushDownFilter, and also clean the code up a bit.