Skip to content

Commit 7d95349

Browse files
jizezhanghsiang-c
authored andcommitted
ci: enforce needless_pass_by_value for datafusion-optimzer (apache#18533)
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#18505. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Enforce clippy `needless_pass_by_value`. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Yes ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> No
1 parent 34a5ccb commit 7d95349

File tree

4 files changed

+24
-21
lines changed

4 files changed

+24
-21
lines changed

datafusion/optimizer/src/analyzer/resolve_grouping_function.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use arrow::datatypes::DataType;
2828
use datafusion_common::config::ConfigOptions;
2929
use datafusion_common::tree_node::{Transformed, TransformedResult, TreeNode};
3030
use datafusion_common::{
31-
internal_datafusion_err, plan_err, Column, DFSchemaRef, Result, ScalarValue,
31+
internal_datafusion_err, plan_err, Column, DFSchema, Result, ScalarValue,
3232
};
3333
use datafusion_expr::expr::{AggregateFunction, Alias};
3434
use datafusion_expr::logical_plan::LogicalPlan;
@@ -74,7 +74,7 @@ fn group_expr_to_bitmap_index(group_expr: &[Expr]) -> Result<HashMap<&Expr, usiz
7474

7575
fn replace_grouping_exprs(
7676
input: Arc<LogicalPlan>,
77-
schema: DFSchemaRef,
77+
schema: &DFSchema,
7878
group_expr: Vec<Expr>,
7979
aggr_expr: Vec<Expr>,
8080
) -> Result<LogicalPlan> {
@@ -139,7 +139,7 @@ fn analyze_internal(plan: LogicalPlan) -> Result<Transformed<LogicalPlan>> {
139139
schema,
140140
..
141141
}) if contains_grouping_function(&aggr_expr) => Ok(Transformed::yes(
142-
replace_grouping_exprs(input, schema, group_expr, aggr_expr)?,
142+
replace_grouping_exprs(input, schema.as_ref(), group_expr, aggr_expr)?,
143143
)),
144144
_ => Ok(Transformed::no(plan)),
145145
})?;

datafusion/optimizer/src/decorrelate_predicate_subquery.rs

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ fn rewrite_inner_subqueries(
136136
Expr::Exists(Exists {
137137
subquery: Subquery { subquery, .. },
138138
negated,
139-
}) => match mark_join(&cur_input, Arc::clone(&subquery), None, negated, alias)? {
139+
}) => match mark_join(&cur_input, &subquery, None, negated, alias)? {
140140
Some((plan, exists_expr)) => {
141141
cur_input = plan;
142142
Ok(Transformed::yes(exists_expr))
@@ -154,13 +154,7 @@ fn rewrite_inner_subqueries(
154154
.map_or(plan_err!("single expression required."), |output_expr| {
155155
Ok(Expr::eq(*expr.clone(), output_expr))
156156
})?;
157-
match mark_join(
158-
&cur_input,
159-
Arc::clone(&subquery),
160-
Some(in_predicate),
161-
negated,
162-
alias,
163-
)? {
157+
match mark_join(&cur_input, &subquery, Some(&in_predicate), negated, alias)? {
164158
Some((plan, exists_expr)) => {
165159
cur_input = plan;
166160
Ok(Transformed::yes(exists_expr))
@@ -275,7 +269,13 @@ fn build_join_top(
275269
};
276270
let subquery = query_info.query.subquery.as_ref();
277271
let subquery_alias = alias.next("__correlated_sq");
278-
build_join(left, subquery, in_predicate_opt, join_type, subquery_alias)
272+
build_join(
273+
left,
274+
subquery,
275+
in_predicate_opt.as_ref(),
276+
join_type,
277+
subquery_alias,
278+
)
279279
}
280280

281281
/// This is used to handle the case when the subquery is embedded in a more complex boolean
@@ -295,8 +295,8 @@ fn build_join_top(
295295
/// TableScan: t2
296296
fn mark_join(
297297
left: &LogicalPlan,
298-
subquery: Arc<LogicalPlan>,
299-
in_predicate_opt: Option<Expr>,
298+
subquery: &LogicalPlan,
299+
in_predicate_opt: Option<&Expr>,
300300
negated: bool,
301301
alias_generator: &Arc<AliasGenerator>,
302302
) -> Result<Option<(LogicalPlan, Expr)>> {
@@ -306,20 +306,20 @@ fn mark_join(
306306
let exists_expr = if negated { !exists_col } else { exists_col };
307307

308308
Ok(
309-
build_join(left, &subquery, in_predicate_opt, JoinType::LeftMark, alias)?
309+
build_join(left, subquery, in_predicate_opt, JoinType::LeftMark, alias)?
310310
.map(|plan| (plan, exists_expr)),
311311
)
312312
}
313313

314314
fn build_join(
315315
left: &LogicalPlan,
316316
subquery: &LogicalPlan,
317-
in_predicate_opt: Option<Expr>,
317+
in_predicate_opt: Option<&Expr>,
318318
join_type: JoinType,
319319
alias: String,
320320
) -> Result<Option<LogicalPlan>> {
321321
let mut pull_up = PullUpCorrelatedExpr::new()
322-
.with_in_predicate_opt(in_predicate_opt.clone())
322+
.with_in_predicate_opt(in_predicate_opt.cloned())
323323
.with_exists_sub_query(in_predicate_opt.is_none());
324324

325325
let new_plan = subquery.clone().rewrite(&mut pull_up).data()?;
@@ -342,7 +342,7 @@ fn build_join(
342342
replace_qualified_name(filter, &all_correlated_cols, &alias).map(Some)
343343
})?;
344344

345-
let join_filter = match (join_filter_opt, in_predicate_opt.clone()) {
345+
let join_filter = match (join_filter_opt, in_predicate_opt.cloned()) {
346346
(
347347
Some(join_filter),
348348
Some(Expr::BinaryExpr(BinaryExpr {
@@ -378,7 +378,7 @@ fn build_join(
378378
// Gather all columns needed for the join filter + predicates
379379
let mut needed = std::collections::HashSet::new();
380380
expr_to_columns(&join_filter, &mut needed)?;
381-
if let Some(ref in_pred) = in_predicate_opt {
381+
if let Some(in_pred) = in_predicate_opt {
382382
expr_to_columns(in_pred, &mut needed)?;
383383
}
384384

datafusion/optimizer/src/lib.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
// Make sure fast / cheap clones on Arc are explicit:
2424
// https://github.com/apache/datafusion/issues/11143
2525
#![deny(clippy::clone_on_ref_ptr)]
26+
// https://github.com/apache/datafusion/issues/18503
27+
#![deny(clippy::needless_pass_by_value)]
28+
#![cfg_attr(test, allow(clippy::needless_pass_by_value))]
2629

2730
//! # DataFusion Optimizer
2831
//!

datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1659,7 +1659,7 @@ impl<S: SimplifyInfo> TreeNodeRewriter for Simplifier<'_, S> {
16591659
.to_string();
16601660
Transformed::yes(Expr::Like(Like {
16611661
pattern: Box::new(to_string_scalar(
1662-
data_type,
1662+
&data_type,
16631663
Some(simplified_pattern),
16641664
)),
16651665
..like
@@ -1971,7 +1971,7 @@ fn as_string_scalar(expr: &Expr) -> Option<(DataType, &Option<String>)> {
19711971
}
19721972
}
19731973

1974-
fn to_string_scalar(data_type: DataType, value: Option<String>) -> Expr {
1974+
fn to_string_scalar(data_type: &DataType, value: Option<String>) -> Expr {
19751975
match data_type {
19761976
DataType::Utf8 => Expr::Literal(ScalarValue::Utf8(value), None),
19771977
DataType::LargeUtf8 => Expr::Literal(ScalarValue::LargeUtf8(value), None),

0 commit comments

Comments
 (0)