Skip to content

Conversation

mbrobbel
Copy link
Member

Which issue does this PR close?

Rationale for this change

Splitting up #8227.

What changes are included in this PR?

Migrate parquet-variant-compute to Rust 2024

Are these changes tested?

CI

Are there any user-facing changes?

Yes

@mbrobbel mbrobbel added this to the 57.0.0 milestone Sep 30, 2025
@github-actions github-actions bot added the parquet-variant parquet-variant* crates label Sep 30, 2025
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.

Looks great to me -- thank you @mbrobbel -- my only concern is that it will conflict with some of the outstanding PRs from @scovich , but I think given the changes it should be manageable

@mbrobbel
Copy link
Member Author

Looks great to me -- thank you @mbrobbel -- my only concern is that it will conflict with some of the outstanding PRs from @scovich , but I think given the changes it should be manageable

We can maybe time this one so it doesn't conflict with open PRs? This one is easy to update/rebase.

@scovich
Copy link
Contributor

scovich commented Oct 2, 2025

I think it's a good time to update+merge this one; I'm tempted to use a let chain in a PR, which requires rust 2024

@alamb
Copy link
Contributor

alamb commented Oct 2, 2025

Let's get it refreshed and in!

@mbrobbel
Copy link
Member Author

mbrobbel commented Oct 3, 2025

I think it's a good time to update+merge this one; I'm tempted to use a let chain in a PR, which requires rust 2024

We need to wait until we can bump the MSRV to 1.88 to get let chains: https://blog.rust-lang.org/2025/06/26/Rust-1.88.0/#let-chains

@alamb alamb merged commit 2d900a4 into apache:main Oct 3, 2025
18 checks passed
@alamb
Copy link
Contributor

alamb commented Oct 3, 2025

Thanks @mbrobbel -- bummer about the let chains. soon ™️

@mbrobbel mbrobbel deleted the parquet-variant-compute-2024 branch October 3, 2025 14:51
@scovich
Copy link
Contributor

scovich commented Oct 3, 2025

We need to wait until we can bump the MSRV to 1.88 to get let chains

Odd... I thought this same series of pull requests had also updated MSRV to 1.88? At least, I was no longer able to compile with older rust versions after pulling latest main and forcing to 1.85:

% cargo clippy --workspace --keep-going --tests
error: rustc 1.85.1 is not supported by the following package:
  [email protected] requires rustc 1.88

@scovich
Copy link
Contributor

scovich commented Oct 3, 2025

Ah, but it's "only" a dev dependency:

% cargo tree -i sysinfo        
sysinfo v0.37.1
[dev-dependencies]
├── parquet v56.2.0 (/Users/ryan.johnson/arrow-rs/parquet)
│   └── parquet_derive v56.2.0 (proc-macro) (/Users/ryan.johnson/arrow-rs/parquet_derive)
│       └── parquet_derive_test v56.2.0 (/Users/ryan.johnson/arrow-rs/parquet_derive_test)
└── parquet v56.2.0 (/Users/ryan.johnson/arrow-rs/parquet)
    └── parquet_derive_test v56.2.0 (/Users/ryan.johnson/arrow-rs/parquet_derive_test)

@scovich
Copy link
Contributor

scovich commented Oct 3, 2025

Aside: When compiling with 1.85, I get a lot of clippy warnings of the form:

warning: the following explicit lifetimes could be elided: 'a
    --> arrow-avro/src/writer/encoder.rs:1330:6
     |
1330 | impl<'a, P: ArrowPrimitiveType + IntervalToDurationParts> DurationEncoder<'a, P> {
     |      ^^                                                                   ^^
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
help: elide the lifetimes
     |
1330 - impl<'a, P: ArrowPrimitiveType + IntervalToDurationParts> DurationEncoder<'a, P> {
1330 + impl<P: ArrowPrimitiveType + IntervalToDurationParts> DurationEncoder<'_, P> {
     |

... which seems very legit, I wonder why clippy 1.90 doesn't flag them?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet-variant parquet-variant* crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants