Skip to content

Minor: remove an unecessary clone in common_sort_prefix_length #72

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

Merged
merged 1 commit into from
Jun 3, 2025
Merged

Minor: remove an unecessary clone in common_sort_prefix_length #72

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
12 changes: 6 additions & 6 deletions datafusion/physical-expr/src/equivalence/properties/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,7 +495,7 @@ impl EquivalenceProperties {
// If the ordering vanishes after normalization, it is satisfied:
return Ok(false);
};
if normal_ordering.len() != self.common_sort_prefix_length(normal_ordering)? {
if normal_ordering.len() != self.common_sort_prefix_length(&normal_ordering)? {
// If the ordering is unsatisfied, replace existing orderings:
self.clear_orderings();
self.add_ordering(ordering_tee);
Expand Down Expand Up @@ -538,7 +538,7 @@ impl EquivalenceProperties {
// If the ordering vanishes after normalization, it is satisfied:
return Ok(true);
};
Ok(normal_ordering.len() == self.common_sort_prefix_length(normal_ordering)?)
Ok(normal_ordering.len() == self.common_sort_prefix_length(&normal_ordering)?)
}

/// Iteratively checks whether the given sort requirement is satisfied by
Expand Down Expand Up @@ -619,10 +619,10 @@ impl EquivalenceProperties {

/// Returns the number of consecutive sort expressions (starting from the
/// left) that are satisfied by the existing ordering.
fn common_sort_prefix_length(&self, normal_ordering: LexOrdering) -> Result<usize> {
fn common_sort_prefix_length(&self, normal_ordering: &LexOrdering) -> Result<usize> {
let full_length = normal_ordering.len();
// Check whether the given ordering is satisfied by constraints:
if self.satisfied_by_constraints_ordering(&normal_ordering) {
if self.satisfied_by_constraints_ordering(normal_ordering) {
// If constraints satisfy all sort expressions, return the full
// length:
return Ok(full_length);
Expand Down Expand Up @@ -661,7 +661,7 @@ impl EquivalenceProperties {
// From the analysis above, we know that `[a ASC]` is satisfied. Then,
// we add column `a` as constant to the algorithm state. This enables us
// to deduce that `(b + c) ASC` is satisfied, given `a` is constant.
let const_expr = ConstExpr::from(element.expr);
let const_expr = ConstExpr::from(Arc::clone(&element.expr));
eq_properties.add_constants(std::iter::once(const_expr))?
}
// All sort expressions are satisfied, return full length:
Expand All @@ -680,7 +680,7 @@ impl EquivalenceProperties {
// If the ordering vanishes after normalization, it is satisfied:
return Ok((vec![], true));
};
let prefix_len = self.common_sort_prefix_length(normal_ordering.clone())?;
Copy link
Author

@alamb alamb Jun 3, 2025

Choose a reason for hiding this comment

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

The point of this PR is to remove this clone

let prefix_len = self.common_sort_prefix_length(&normal_ordering)?;
let flag = prefix_len == normal_ordering.len();
let mut sort_exprs: Vec<_> = normal_ordering.into();
if !flag {
Expand Down