Skip to content
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.

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

Enable clone_on_ref_ptr clippy lint on physical-expr-common crate #13295

Merged
merged 3 commits into from
Nov 7, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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: 8 additions & 4 deletions datafusion/physical-expr-common/src/binary_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,14 @@ where
let batch_hashes = &mut self.hashes_buffer;
batch_hashes.clear();
batch_hashes.resize(values.len(), 0);
create_hashes(&[values.clone()], &self.random_state, batch_hashes)
// hash is supported for all types and create_hashes only
// returns errors for unsupported types
.unwrap();
create_hashes(
&[Arc::<dyn Array>::clone(values)],
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really an explicit type here?

Copy link
Contributor Author

@getChan getChan Nov 7, 2024

Choose a reason for hiding this comment

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

maybe not. removed explicit type

&self.random_state,
batch_hashes,
)
// hash is supported for all types and create_hashes only
// returns errors for unsupported types
.unwrap();

// step 2: insert each value into the set, if not already present
let values = values.as_bytes::<B>();
Expand Down
12 changes: 8 additions & 4 deletions datafusion/physical-expr-common/src/binary_view_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,14 @@ where
let batch_hashes = &mut self.hashes_buffer;
batch_hashes.clear();
batch_hashes.resize(values.len(), 0);
create_hashes(&[values.clone()], &self.random_state, batch_hashes)
// hash is supported for all types and create_hashes only
// returns errors for unsupported types
.unwrap();
create_hashes(
&[Arc::<dyn Array>::clone(values)],
&self.random_state,
batch_hashes,
)
// hash is supported for all types and create_hashes only
// returns errors for unsupported types
.unwrap();

// step 2: insert each value into the set, if not already present
let values = values.as_byte_view::<B>();
Expand Down
3 changes: 3 additions & 0 deletions datafusion/physical-expr-common/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@
// specific language governing permissions and limitations
// under the License.

// Make cheap clones clear: https://github.com/apache/datafusion/issues/11143
#![deny(clippy::clone_on_ref_ptr)]

//! Physical Expr Common packages for [DataFusion]
//! This package contains high level PhysicalExpr trait
//!
Expand Down
6 changes: 5 additions & 1 deletion datafusion/physical-expr-common/src/tree_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,11 @@ impl<T> ExprContext<T> {
}

pub fn update_expr_from_children(mut self) -> Result<Self> {
let children_expr = self.children.iter().map(|c| c.expr.clone()).collect();
let children_expr = self
.children
.iter()
.map(|c| Arc::<dyn PhysicalExpr>::clone(&c.expr))
.collect();
self.expr = with_new_children_if_necessary(self.expr, children_expr)?;
Ok(self)
}
Expand Down
4 changes: 3 additions & 1 deletion datafusion/physical-expr-common/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,9 @@ pub fn scatter(mask: &BooleanArray, truthy: &dyn Array) -> Result<ArrayRef> {
pub fn reverse_order_bys(order_bys: &LexOrdering) -> LexOrdering {
order_bys
.iter()
.map(|e| PhysicalSortExpr::new(e.expr.clone(), !e.options))
.map(|e| {
PhysicalSortExpr::new(Arc::<dyn PhysicalExpr>::clone(&e.expr), !e.options)
})
.collect()
}

Expand Down
Loading