Skip to content

Conversation

@carlosahs
Copy link
Contributor

@carlosahs carlosahs commented Dec 7, 2025

Which issue does this PR close?

Rationale for this change

Please refer to #18881.

What changes are included in this PR?

  • Added #![deny(clippy::allow_attributes)] to the following crates:

    • datafusion-physical-expr-adapter.
    • datafusion-physical-expr-common.
    • datafusion-physical-optimizer.
  • Removed #[allow(deprecated)] from

    #[allow(deprecated)]
    fn optimize(
    &self,
    plan: Arc<dyn ExecutionPlan>,
    config: &ConfigOptions,
    ) -> Result<Arc<dyn ExecutionPlan>> {
    Ok(
    push_down_filters(&Arc::clone(&plan), vec![], config, self.phase)?
    .updated_node
    .unwrap_or(plan),
    )
    }
    as lint is only fulfilled on usage of #[deprecated] items, thus when #[expect(deprecated)] is used instead of #[allow(deprecated)] it results in error: this lint expectation is unfulfilled.

  • Kept #[allow(clippy::only_used_in_recursion)] in

    #[cfg_attr(feature = "recursive_protection", recursive::recursive)]
    #[allow(clippy::only_used_in_recursion)] // See https://github.com/rust-lang/rust-clippy/issues/14566
    fn optimize_plan(
    &self,
    plan: Arc<dyn ExecutionPlan>,
    context: &OptimizerContext,
    ) -> Result<Arc<dyn ExecutionPlan>> {
    if let Some(partial_agg_exec) = take_optimizable(&*plan) {
    let partial_agg_exec = partial_agg_exec
    .as_any()
    .downcast_ref::<AggregateExec>()
    .expect("take_optimizable() ensures that this is a AggregateExec");
    let stats = partial_agg_exec.input().partition_statistics(None)?;
    let mut projections = vec![];
    for expr in partial_agg_exec.aggr_expr() {
    let field = expr.field();
    let args = expr.expressions();
    let statistics_args = StatisticsArgs {
    statistics: &stats,
    return_type: field.data_type(),
    is_distinct: expr.is_distinct(),
    exprs: args.as_slice(),
    };
    if let Some((optimizable_statistic, name)) =
    take_optimizable_value_from_statistics(&statistics_args, expr)
    {
    projections.push(ProjectionExpr {
    expr: expressions::lit(optimizable_statistic),
    alias: name.to_owned(),
    });
    } else {
    // TODO: we need all aggr_expr to be resolved (cf TODO fullres)
    break;
    }
    }
    // TODO fullres: use statistics even if not all aggr_expr could be resolved
    if projections.len() == partial_agg_exec.aggr_expr().len() {
    // input can be entirely removed
    Ok(Arc::new(ProjectionExec::try_new(
    projections,
    Arc::new(PlaceholderRowExec::new(plan.schema())),
    )?))
    } else {
    plan.map_children(|child| {
    self.optimize_plan(child, context).map(Transformed::yes)
    })
    .data()
    }
    } else {
    plan.map_children(|child| {
    self.optimize_plan(child, context).map(Transformed::yes)
    })
    .data()
    }
    }
    as #[expect(clippy::only_used_in_recursion)] resulted in error: this lint expectation is unfulfilled when running sh ci/scripts/rust_clippy.sh but was successful when running cargo clippy -p datafusion-physical-optimizer -- -D warning. Ref: New lint clippy::allow_attributes #18881 (comment).

    For now, easiest solution is to locally expect #[expect(clippy::allow_attributes)].

Are these changes tested?

Successfully ran sh ci/scripts/rust_clippy.sh.

Are there any user-facing changes?

No.

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates optimizer Optimizer rules labels Dec 7, 2025
@Jefffrey Jefffrey mentioned this pull request Dec 8, 2025
38 tasks
@Jefffrey Jefffrey added this pull request to the merge queue Dec 8, 2025
Merged via the queue into apache:main with commit 41f7137 Dec 8, 2025
35 checks passed
@Jefffrey
Copy link
Contributor

Jefffrey commented Dec 8, 2025

Thanks @carlosahs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

optimizer Optimizer rules physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants