Skip to content

chore(CI) Upgrade toolchain to Rust-1.87 #16068

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 2 commits into from
May 19, 2025

Conversation

kadai0308
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

Improve build times.

What changes are included in this PR?

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

The main changes including:

  • datafusion/common/src/stats.rs:355:9 and the following related changes
359 -             Present(ColumnStatistics),
359 +             Present(Box<ColumnStatistics>),
  • datafusion/expr/src/planner.rs:315:1 and the following related changes
317 -     Planned(Expr),
317 +     Planned(Box<Expr>),
  • datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs:523:1 and the following related changes
529 -     SimplifyRuntimeError(DataFusionError, Expr),
529 +     SimplifyRuntimeError(DataFusionError, Box<Expr>),
  • datafusion/physical-plan/src/aggregates/mod.rs:349:1 and the following related changes
351 -     GroupedHash(GroupedHashAggregateStream),
351 +     GroupedHash(Box<GroupedHashAggregateStream>),
  • datafusion/datasource-avro/src/avro_to_arrow/arrow_array_reader.rs:941:29
941 | fn resolve_u8(v: &Value) -> AvroResult<u8> {
    |                             ^^^^^^^^^^^^^^ the `Err`-variant is at least 256 bytes

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 optimizer Optimizer rules common Related to common crate functions Changes to functions implementation datasource Changes to the datasource crate labels May 16, 2025
@kadai0308 kadai0308 marked this pull request as draft May 16, 2025 15:55
@kadai0308 kadai0308 force-pushed the chore/update-workspace-CI-to-rust-1.87 branch from c111d03 to df88591 Compare May 16, 2025 15:55
@kadai0308 kadai0308 marked this pull request as ready for review May 16, 2025 15:56
@kadai0308 kadai0308 force-pushed the chore/update-workspace-CI-to-rust-1.87 branch from df88591 to b69ad61 Compare May 16, 2025 16:03
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 for this PR @kadai0308 -- very helpful

I am not sure about the implications of Boxing all the variants -- I worry it will just add additional overhead for planning and API churn (though I'll run some benchmarks0

Rather than adding new Box would you be willing instead to add in the #[clippy(allow(..)) annotations instead for errors about mismatched enum sizes?

I think this would allow us to unblock the upgrade to 1.87 and we can then consider how to fix the enum variant size mismatch as a follow on

For example, one thing I would like to consider is making Expr smaller, for example, this PR: #14366

@kadai0308 kadai0308 force-pushed the chore/update-workspace-CI-to-rust-1.87 branch from b69ad61 to 5bd7dc8 Compare May 17, 2025 06:59
@github-actions github-actions bot removed the functions Changes to functions implementation label May 17, 2025
@kadai0308
Copy link
Contributor Author

Thank you for this PR @kadai0308 -- very helpful

I am not sure about the implications of Boxing all the variants -- I worry it will just add additional overhead for planning and API churn (though I'll run some benchmarks0

Rather than adding new Box would you be willing instead to add in the #[clippy(allow(..)) annotations instead for errors about mismatched enum sizes?

I think this would allow us to unblock the upgrade to 1.87 and we can then consider how to fix the enum variant size mismatch as a follow on

For example, one thing I would like to consider is making Expr smaller, for example, this PR: #14366

Thanks for the feedback! Yeah, I agree — adding Box to some enum variants and the associated changes could make the codebase and API more unstable and fragile.
I just update the PR with #[allow(clippy::large_enum_variant)].

Another notable change is related to this warning:

941 | fn resolve_u8(v: &Value) -> AvroResult<u8> {
    |                             ^^^^^^^^^^^^^^ the `Err`-variant is at least 256 bytes

It seems we could return Option instead of AvroResult, because

        Value::Array(items) => Ok(Value::Bytes(
            items
                .iter()
                .map(resolve_u8)
                .collect::<Result<Vec<_>, _>>()
                .ok()?,
        )),

If I understand correctly, .ok()? effectively drops the error from AvroResult, making Option sufficient in this context.

@kadai0308 kadai0308 force-pushed the chore/update-workspace-CI-to-rust-1.87 branch from 5bd7dc8 to 3edb3e9 Compare May 17, 2025 07:23
@kadai0308 kadai0308 force-pushed the chore/update-workspace-CI-to-rust-1.87 branch from 3edb3e9 to 9d3f1bb Compare May 18, 2025 14:09
@github-actions github-actions bot added the physical-plan Changes to the physical-plan crate label May 19, 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.

Thank you @kadai0308 and @findepi

@alamb alamb merged commit 2ea1e95 into apache:main May 19, 2025
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Related to common crate datasource Changes to the datasource crate logical-expr Logical plan and expressions optimizer Optimizer rules physical-plan Changes to the physical-plan crate sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update workspace / CI to Rust 1.87
3 participants