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.

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

Fix up clippy for Rust 1.78 #5710

Merged
merged 2 commits into from
May 4, 2024
Merged

Fix up clippy for Rust 1.78 #5710

merged 2 commits into from
May 4, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented May 2, 2024

Which issue does this PR close?

Closes #.

Rationale for this change

Rust 1.78 was released today and it includes new clippy lints

What changes are included in this PR?

Fix the lints

Are there any user-facing changes?

No

@github-actions github-actions bot added parquet Changes to the parquet crate arrow Changes to the arrow crate labels May 2, 2024
@@ -435,11 +435,20 @@ impl Parser for Float64Type {
}
}

/// This API is only stable since 1.70 so can't use it when current MSRV is lower
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 simply copy/pasted from is_some_and https://docs.rs/parquet/latest/src/parquet/file/metadata.rs.html#594-596

Otherwise clippy complains like

error: current MSRV (Minimum Supported Rust Version) is `1.62.0` but this item is stable since `1.70.0`
   --> arrow-json/src/writer/encoder.rs:163:56
    |
163 |             let is_null = field_encoder.nulls.as_ref().is_some_and(|n| n.is_null(idx));
    |                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#incompatible_msrv
    = note: `-D clippy::incompatible-msrv` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::incompatible_msrv)]`

@@ -40,7 +40,13 @@ struct Args {
arrow: String,
#[clap(short, long, help("Path to JSON file"))]
json: String,
#[clap(value_enum, short, long, default_value_t = Mode::Validate, help="Mode of integration testing tool")]
#[clap(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

apparently default_value_t (parsed value) is only available in Rust 1.70

I verified this is the right value (SCREAMING_SNAKE_CASE 🐍 ) like this:

cargo run --bin arrow-json-integration-test

@@ -155,12 +155,21 @@ struct StructArrayEncoder<'a> {
explicit_nulls: bool,
}

/// This API is only stable since 1.70 so can't use it when current MSRV is lower
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

@@ -1172,13 +1172,6 @@ fn compare_greater<T: ParquetValueType>(descr: &ColumnDescriptor, a: &T, b: &T)
// https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/column/values/factory/DefaultV1ValuesWriterFactory.java
// https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/column/values/factory/DefaultV2ValuesWriterFactory.java

/// Trait to define default encoding for types, including whether or not the type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unused

@@ -47,12 +47,13 @@ pub trait Encoder<T: DataType>: Send {
/// identified by `valid_bits`.
///
/// Returns the number of non-null values encoded.
#[cfg(test)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only used in tests apparently

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Can we make sure these aren't exposed publicly

@@ -435,11 +435,20 @@ impl Parser for Float64Type {
}
}

/// This API is only stable since 1.70 so can't use it when current MSRV is lower
#[inline(always)]
pub fn is_some_and<T>(opt: Option<T>, f: impl FnOnce(T) -> bool) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no it doesn't. I will fix

@alamb
Copy link
Contributor Author

alamb commented May 3, 2024

CI failed with some sort of linker error -- I have retriggered it

@alamb
Copy link
Contributor Author

alamb commented May 4, 2024

I believe the archery integration test failure is not related to this PR -- I filed #5719 to track it

@alamb alamb merged commit b3f06f6 into apache:master May 4, 2024
26 of 27 checks passed
@alamb
Copy link
Contributor Author

alamb commented May 4, 2024

Merging to keep things moving along

@alamb alamb deleted the alamb/clippy branch May 4, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants