Skip to content

Conversation

@Gohlub
Copy link

@Gohlub Gohlub commented Nov 7, 2025

Which issue does this PR close?

Rationale for this change

Followed suggestions to not update any public-facing APIs and put the lint rule in the appropriate spot.

What changes are included in this PR?

  • Add #![deny(clippy::needless_pass_by_value)] and #![cfg_attr(test, allow(clippy::needless_pass_by_value))] to lib.rs.
  • Add #[allow(clippy::needless_pass_by_value)] to public functions
  • fix rewrite_in_terms_of_projection() and get_exprs_except_skipped() to use references per the lint suggestion

Are these changes tested?

Yes, though the same test failed even without changes to the public APIs:
test expr_rewriter::order_by::test::rewrite_sort_cols_by_agg_alias ... FAILED
I'll append the logs for your convenience:

failures:

---- expr_rewriter::order_by::test::rewrite_sort_cols_by_agg_alias stdout ----
running: 'c1 --> c1  -- column *named* c1 that came out of the projection, (not t.c1)'
running: 'min(c2) --> "min(c2)" -- (column *named* "min(t.c2)"!)'

thread 'expr_rewriter::order_by::test::rewrite_sort_cols_by_agg_alias' (27524241) panicked at datafusion/expr/src/expr_rewriter/order_by.rs:308:13:
assertion `left == right` failed: 

input:Sort { expr: AggregateFunction(AggregateFunction { func: AggregateUDF { inner: Min { name: "min", signature: Signature { type_signature: VariadicAny, volatility: Immutable, parameter_names: None } } }, params: AggregateFunctionParams { args: [Column(Column { relation: None, name: "c2" })], distinct: false, filter: None, order_by: [], null_treatment: None } }), asc: true, nulls_first: true }
rewritten:Sort { expr: Column(Column { relation: None, name: "min(t.c2)" }), asc: true, nulls_first: true }
expected:Sort { expr: Column(Column { relation: Some(Bare { table: "min(t" }), name: "c2)" }), asc: true, nulls_first: true }

  left: Sort { expr: Column(Column { relation: None, name: "min(t.c2)" }), asc: true, nulls_first: true }
 right: Sort { expr: Column(Column { relation: Some(Bare { table: "min(t" }), name: "c2)" }), asc: true, nulls_first: true }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    expr_rewriter::order_by::test::rewrite_sort_cols_by_agg_alias

Are there any user-facing changes?

No, all modification were constrained to internal APIs.

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Nov 7, 2025
Copy link
Contributor

@2010YOUY01 2010YOUY01 left a comment

Choose a reason for hiding this comment

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

Thank you! This is great.

It should be good to go after CI passes. It seem there is no test failing in CI 🤔 For the formatter failure you can reproduce with ./dev/rust_lint.sh locally, and fix accordingly.

@Gohlub
Copy link
Author

Gohlub commented Nov 8, 2025

Should I try and squash the changes/merges for a cleaner history or it's fine to leave it as is?

@2010YOUY01
Copy link
Contributor

Should I try and squash the changes/merges for a cleaner history or it's fine to leave it as is?

It's okay to keep it as is, since this PR is relatively small and it's easy to see the whole change -- It's helpful to cleanup the commits to several atomic units for larger PRs.

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

Labels

logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enforce lint rule clippy::needless_pass_by_value to datafusion-expr

2 participants