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
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 ?
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
| @@ -802,11 +802,11 @@ impl OptimizerRule for PushDownFilter { | |||
| replace_map.insert(expr.display_name()?, expr.clone()); | |||
| } | |||
| 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
| @@ -829,12 +829,12 @@ impl OptimizerRule for PushDownFilter { | |||
| } | |||
| 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
| @@ -861,16 +861,12 @@ impl OptimizerRule for PushDownFilter { | |||
| .collect(); | |||
| let new_predicate: Vec<Expr> = zip | |||
| .filter(|(_, res)| res != &TableProviderFilterPushDown::Exact) | |||
| .map(|(pred, _)| (*pred).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 |
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.