Skip to content

Avoid some clones using impl IntoIterator #17831

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Avoid some clones using impl IntoIterator #17831

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions datafusion/optimizer/src/push_down_filter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,14 +530,17 @@ fn push_down_join(
parent_predicate: Option<&Expr>,
) -> Result<Transformed<LogicalPlan>> {
// 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 =
parent_predicate.map_or_else(Vec::new, |pred| split_conjunction(pred));

// Extract conjunctions from the JOIN's ON filter, if present.
let on_filters = join
.filter
.as_ref()
.map_or_else(Vec::new, |filter| split_conjunction_owned(filter.clone()));
.map_or_else(Vec::new, |filter| split_conjunction(filter));

let predicates: Vec<Expr> = predicates.into_iter().cloned().collect();
Copy link
Contributor

@comphead comphead Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alamb

cloned() on iter calls a clone under the hood for each element?

https://doc.rust-lang.org/beta/std/iter/trait.Iterator.html#method.cloned

I'm trying to figure out the benefit

Copy link
Member

@findepi findepi Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The split_conjunction_owned -> split_conjunction change above avoids cloning the pred components.
But any benefit seems to be indeed voided by cloning here, which we execute unconditionally.

Copy link
Contributor Author

@alamb alamb Sep 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I should have been clearer.

This code is splitting an expresision like

AND
  AND
    expr1
    expr2
  expr3

The cloned call still clones expr1, expr2, and expr3 but it doesn't clone the ANDs.

However, I agree that the benefit seems minor - I was more interested in alternate approaches than defining AsRef for Expr #17819

#17819 also included this improvement and I wanted to show it was also possible in another way too

I am happy to close this PR too

let on_filters: Vec<Expr> = on_filters.into_iter().cloned().collect();

// Are there any new join predicates that can be inferred from the filter expressions?
let inferred_join_predicates =
Expand All @@ -563,10 +566,10 @@ fn push_down_join(
/// * `on_filters` filters from the join ON clause that have not already been
/// identified as join predicates
///
fn infer_join_predicates(
fn infer_join_predicates<'a>(
join: &Join,
predicates: &[Expr],
on_filters: &[Expr],
predicates: impl IntoIterator<Item = &'a Expr>,
on_filters: impl IntoIterator<Item = &'a Expr>,
) -> Result<Vec<Expr>> {
// Only allow both side key is column.
let join_col_keys = join
Expand Down Expand Up @@ -650,9 +653,9 @@ impl InferredPredicates {
///
/// * `inferred_predicates` the inferred results
///
fn infer_join_predicates_from_predicates(
fn infer_join_predicates_from_predicates<'a>(
join_col_keys: &[(&Column, &Column)],
predicates: &[Expr],
predicates: impl IntoIterator<Item = &'a Expr>,
inferred_predicates: &mut InferredPredicates,
) -> Result<()> {
infer_join_predicates_impl::<true, true>(
Expand All @@ -674,10 +677,10 @@ fn infer_join_predicates_from_predicates(
///
/// * `inferred_predicates` the inferred results
///
fn infer_join_predicates_from_on_filters(
fn infer_join_predicates_from_on_filters<'a>(
join_col_keys: &[(&Column, &Column)],
join_type: JoinType,
on_filters: &[Expr],
on_filters: impl IntoIterator<Item = &'a Expr>,
inferred_predicates: &mut InferredPredicates,
) -> Result<()> {
match join_type {
Expand Down Expand Up @@ -721,11 +724,12 @@ fn infer_join_predicates_from_on_filters(
/// be inferred from the right table related predicate
///
fn infer_join_predicates_impl<
'a,
const ENABLE_LEFT_TO_RIGHT: bool,
const ENABLE_RIGHT_TO_LEFT: bool,
>(
join_col_keys: &[(&Column, &Column)],
input_predicates: &[Expr],
input_predicates: impl IntoIterator<Item = &'a Expr>,
inferred_predicates: &mut InferredPredicates,
) -> Result<()> {
for predicate in input_predicates {
Expand Down