Skip to content

Upgrade toolchain to Rust-1.86 #15625

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

Merged
merged 1 commit into from
Apr 8, 2025

Conversation

jsai28
Copy link
Contributor

@jsai28 jsai28 commented Apr 7, 2025

Which issue does this PR close?

Closes #15619.

Rationale for this change

Improve build times.

What changes are included in this PR?

Updating toolchain to Rust 1.86 and fixing associated errors and warnings.

The main changes were doc indentation updates and changing repeat to use repeat_n. There are few other changes such as using .ok() instead of manually implementing it, using is_empty() instead of checking if length is 0, and others similar to this.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate proto Related to proto crate functions Changes to functions implementation datasource Changes to the datasource crate ffi Changes to the ffi crate labels Apr 7, 2025
@jsai28 jsai28 marked this pull request as ready for review April 7, 2025 19:02
Copy link
Contributor

@DerGut DerGut left a comment

Choose a reason for hiding this comment

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

I verified the clippy warnings I am getting locally and they match exactly 👍


Interestingly, when I run clippy on the datafusion/physical-optimizer crate:

cargo clippy --manifest-path datafusion/physical-optimizer/Cargo.toml

I get a warning.

warning: parameter is only used in recursion
  --> datafusion/physical-optimizer/src/aggregate_statistics.rs:48:9
   |
48 |         config: &ConfigOptions,
   |         ^^^^^^ help: if this is intentional, prefix it with an underscore: `_config`
   |
note: parameter used here
  --> datafusion/physical-optimizer/src/aggregate_statistics.rs:86:42
   |
86 |                     self.optimize(child, config).map(Transformed::yes)
   |                                          ^^^^^^
...
91 |             plan.map_children(|child| self.optimize(child, config).map(Transformed::yes))
   |                                                            ^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
   = note: `#[warn(clippy::only_used_in_recursion)]` on by default

This warning also shows up with v1.85.0 so it shouldn't be relevant for this PR.

@alamb
Copy link
Contributor

alamb commented Apr 8, 2025

Thanks @DerGut

Interestingly, when I run clippy on the datafusion/physical-optimizer crate:

I actually found it fails for me on rust 1.85 as well:

(venv) andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$ cargo clippy -p datafusion-physical-optimizer
warning: parameter is only used in recursion
  --> datafusion/physical-optimizer/src/aggregate_statistics.rs:48:9
   |
48 |         config: &ConfigOptions,
   |         ^^^^^^ help: if this is intentional, prefix it with an underscore: `_config`
   |
note: parameter used here
  --> datafusion/physical-optimizer/src/aggregate_statistics.rs:86:42
   |
86 |                     self.optimize(child, config).map(Transformed::yes)
   |                                          ^^^^^^
...
91 |             plan.map_children(|child| self.optimize(child, config).map(Transformed::yes))
   |                                                            ^^^^^^
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
   = note: `#[warn(clippy::only_used_in_recursion)]` on by default

warning: `datafusion-physical-optimizer` (lib) generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.31s
(venv) andrewlamb@Andrews-MacBook-Pro-2:~/Software/datafusion$

Any chance you could make a follow PR to fix this?

Copy link
Contributor

@alamb alamb 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 @jsai28 -- this looks great. Much appreciated

@alamb
Copy link
Contributor

alamb commented Apr 8, 2025

Since this is non controversial let's upgrade and see how it works

FYI @logan-keede -- maybe we can see how much this helps the build speed

@alamb alamb merged commit 9d2f049 into apache:main Apr 8, 2025
28 checks passed
@DerGut
Copy link
Contributor

DerGut commented Apr 8, 2025

Yes, I can take a look at the only used in recursion warning 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate core Core DataFusion crate datasource Changes to the datasource crate ffi Changes to the ffi crate functions Changes to functions implementation logical-expr Logical plan and expressions optimizer Optimizer rules physical-expr Changes to the physical-expr crates proto Related to proto crate sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade toolchain to Rust 1.86
3 participants