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.

Sign up for GitHub

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

Jump to bottom

Remove unused import detected by nightly rust #5477

Merged
merged 7 commits into from
Mar 11, 2024
Merged

Remove unused import detected by nightly rust #5477

merged 7 commits into from
Mar 11, 2024

Conversation

XiangpengHao
Copy link
Contributor

Which issue does this PR close?

Closes #5476.

Rationale for this change

With this pr been merged to nightly rust: rust-lang/rust#117772
The tracking for unused import has been improved, resulting numerous compiler warnings when building arrow-rs with nightly rust.

This PR simply remove them.

Note that there're many other linting problems as well in nightly rust, this pr only fix the problem with unused import, considering that the changes are quite large already.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate parquet-derive labels Mar 6, 2024
@XiangpengHao
Copy link
Contributor Author

hmm, the CI failed with errors like

error: use of deprecated associated function chrono::TimeDelta::milliseconds: Use TimeDelta::try_milliseconds instead

It does not seem to relate to the changes

@XiangpengHao
Copy link
Contributor Author

Ok the latest CI uses chrono v0.4.35 while previous ci uses v0.4.34.

It turns out that they release the new version a few hours ago and deprecated a few functions used by arrow-rs: https://github.com/chronotope/chrono/releases/tag/v0.4.35

@tustvold
Copy link
Contributor

tustvold commented Mar 7, 2024

Should be fixed if you merge master

@tustvold tustvold mentioned this pull request Mar 9, 2024
Closed
@XiangpengHao
Copy link
Contributor Author

Can @tustvold you take a look at make_fixed_len_byte_array_reader in the bench file? The function is from array_reader and is only used in the benchmark. I can't just remove the export as it will fail the benchmark, so I just suppressed the warning. Not sure it is the right way to do...

tustvold
tustvold reviewed Mar 10, 2024
View reviewed changes
parquet/src/compression.rs
@@ -441,8 +437,6 @@ mod lz4_codec {
}
}
}
#[cfg(any(feature = "lz4", test))]
pub use lz4_codec::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

This removal seems incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried with lz4 on and off, it seems that both are not used.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this is a public export when the experimental flag is enabled

Copy link
Contributor Author

@XiangpengHao XiangpengHao Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The compression.rs is not pub, so the pub use is scoped to this crate only, which is not being used. Do we want to make compression pub instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is public when the experimental feature is enabled

https://github.com/apache/arrow-rs/blob/master/parquet/src/lib.rs#L124

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah you are right! I didn't see two experimental macro there...

parquet/src/arrow/array_reader/mod.rs Outdated Show resolved Hide resolved
@tustvold tustvold merged commit aad42b5 into apache:master Mar 11, 2024
27 checks passed
@tustvold
Copy link
Contributor

Thank you

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

@tustvold tustvold tustvold left review comments

Assignees
No one assigned
Labels
arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate parquet Changes to the parquet crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unused import in nightly rust
2 participants
@XiangpengHao @tustvold