-
Notifications
You must be signed in to change notification settings - Fork 1k
Update MSRV to 1.85 and migrate to Rust 2024 #8227
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mbrobbel
I found this easier to review with whitespace blind diff on: https://github.com/apache/arrow-rs/pull/8227/files?w=1
I reviewed the changes up until the parquet crates I think they look great and make the code easier to read and maintain but I worry about my ability to review it in detail. However, I ran out of time.
The only real concern I have is to ensure that this change remain backwards compatible with existing users / rust versions? Given the MSRV checks I think it will, but we might want to double check (maybe we can try to update DataFusion to use this PR to see)
Would it be possible to split the PR so it is easier to review? Some ideas
- just change
rustfmt.toml
file to get the formatting only changes? I tried it locally and it didn't seem to change anything - Change the edition for only a few crates at a time (arrow, arrow-schema in one PR, then
parquet
in the next, etc)
BTW I think this PR is likely to attract many merge conflicts, so it probably makes sense to get it merged asap
/// +---+---+"#); | ||
/// ``` | ||
pub fn pretty_format_batches(results: &[RecordBatch]) -> Result<impl Display, ArrowError> { | ||
pub fn pretty_format_batches(results: &[RecordBatch]) -> Result<impl Display + use<>, ArrowError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the implication of this change for users of this crate? I assume it is a no-op but I am not familiar with the use
syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relevant change is documented here https://doc.rust-lang.org/edition-guide/rust-2024/rpit-lifetime-capture.html
In Rust 2021 and earlier editions, when the use<..> bound is not present, generic lifetime parameters are only captured when they appear syntactically within a bound in RPIT opaque types in the signature of bare functions and associated functions and methods within inherent impls. However, starting in Rust 2024, these in-scope generic lifetime parameters are unconditionally captured.
The returned type does not capture the lifetime of the recordbatch slice, so use<>
indicates this.
For users this means that the following still compiles in 2024 Rust:
let record_batches: &[RecordBatch] = ...;
let pretty = pretty_format_batches(record_batches)?;
drop(record_batches);
dbg!(pretty);
Without use<>
in 2024 Rust rustc
would complain about record_batches
still being borrowed when dropped.
arrow-ipc/src/gen/Schema.rs
Outdated
unsafe fn follow(buf: &'a [u8], loc: usize) -> Self::Inner { | ||
let b = flatbuffers::read_scalar_at::<i16>(buf, loc); | ||
Self(b) | ||
unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this file is automatically generated by flatbuffers compiler, can we avoid these changes (maybe by suppressing whatever compiler lint / or hard coding this file to use the old edition?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the changes and updated regen.sh
to handle the gen
keyword, and after regenaration it seems flatc
now adds the required unsafe
.
The 2024 edition is supported by rustc >=1.85 so with the msrv bump it should be fine.
I reverted the formatting changes. It's now using 2021 edition for rustfmt. We can update in a different PR.
Let me know if you still want me to do this.
We have to wait until #7836. |
BTW I am worried about this PR causing a lot of merge conflicts, especially for my push decoder PR:
|
No we don't. I'll split this up: |
Which issue does this PR close?
Rationale for this change
Following https://github.com/apache/arrow-rs?tab=readme-ov-file#rust-version-compatibility-policy.
What changes are included in this PR?
unsafe_op_in_unsafe_fn
errors (https://doc.rust-lang.org/edition-guide/rust-2024/unsafe-op-in-unsafe-fn.html)gen
keyword addition (https://doc.rust-lang.org/edition-guide/rust-2024/gen-keyword.html)arrow::util::pretty::pretty_format_batches
arrow::util::pretty::pretty_format_batches_with_schema
arrow::util::pretty::pretty_format_columns
arrow::util::pretty::pretty_format_batches_with_options
arrow::util::pretty::pretty_format_columns_with_options
Use Rustfmt 2024 edition (https://doc.rust-lang.org/edition-guide/rust-2024/rustfmt-style-edition.html)Are these changes tested?
CI
Are there any user-facing changes?
Yes:
arrow::util::pretty
methods