Skip to content

chore: update denpendencies, fix ci #1106

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

Closed
wants to merge 1 commit into from

Conversation

Lordworms
Copy link

Which issue does this PR close?

  • Closes #.

What changes are included in this PR?

Are these changes tested?

@Lordworms
Copy link
Author

cc @liurenjie1024 @Fokko @ZENOTME

Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

fix ci

What problem are we fixing?

@@ -30,7 +30,7 @@ concurrency:
cancel-in-progress: true

env:
rust_msrv: "1.77.1"
rust_msrv: "1.82.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can bump this without any consensus from the community.

Copy link
Author

Choose a reason for hiding this comment

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

We need to upgrade datafusion to 46.0 and the minimal requirement for rust is 1.82

Copy link
Member

Choose a reason for hiding this comment

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

For datafusion integration, we don't need to update MSRV.

Copy link
Member

@xxchan xxchan Mar 19, 2025

Choose a reason for hiding this comment

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

We definitely do not need to update MSRV here.

Whether or not to update the nightly toolchain may be discussed. What compile errors did you see with previous nightly version?

Copy link
Author

Choose a reason for hiding this comment

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

Got some clippy error, if we don't need to update MSRV here, I can restore the changes.

Copy link
Member

Choose a reason for hiding this comment

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

Can you paste the concrete clippy error message here?

Copy link
Author

Choose a reason for hiding this comment

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

Some clippy error like

warning: large size difference between variants
   --> crates/iceberg/src/catalog/mod.rs:364:1
    |
364 | /   pub enum TableUpdate {
365 | |       /// Upgrade table's format version
366 | |       #[serde(rename_all = "kebab-case")]
367 | |       UpgradeFormatVersion {
...   |
379 | | /     AddSchema {
380 | | |         /// The schema to add.
381 | | |         schema: Schema,
382 | | |     },
    | | |_____- the largest variant contains at least 528 bytes
...   |
414 | | /     AddSnapshot {
415 | | |         /// Snapshot to add.
416 | | |         #[serde(deserialize_with = "deserialize_snapshot")]
417 | | |         snapshot: Snapshot,
418 | | |     },
    | | |_____- the second-largest variant contains at least 128 bytes
...   |
484 | |       },
485 | |   }
    | |___^ the entire enum is at least 528 bytes
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
    = note: `#[warn(clippy::large_enum_variant)]` on by default
help: consider boxing the large fields to reduce the total size of the enum
    |
381 |         schema: Box<Schema>,
    |                 ~~~~~~~~~~~

warning: large size difference between variants
   --> crates/iceberg/src/catalog/mod.rs:753:1
    |
753 | /   pub enum ViewUpdate {
754 | |       /// Assign a new UUID to the view
755 | |       #[serde(rename_all = "kebab-case")]
756 | |       AssignUuid {
...   |
768 | | /     AddSchema {
769 | | |         /// The schema to add.
770 | | |         schema: Schema,
771 | | |         /// The last column id of the view.
772 | | |         last_column_id: Option<i32>,
773 | | |     },
    | | |_____- the largest variant contains at least 536 bytes
...   |
796 | | /     AddViewVersion {
797 | | |         /// The view version to add.
798 | | |         view_version: ViewVersion,
799 | | |     },
    | | |_____- the second-largest variant contains at least 136 bytes
...   |
805 | |       },
806 | |   }
    | |___^ the entire enum is at least 536 bytes
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
help: consider boxing the large fields to reduce the total size of the enum
    |
770 |         schema: Box<Schema>,
    |                 ~~~~~~~~~~~

warning: using `map` over `inspect`
  --> crates/iceberg/src/writer/file_writer/track_writer.rs:45:36
   |
45 |         self.inner.write(bs).await.map(|v| {
   |                                    ^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#manual_inspect
   = note: `#[warn(clippy::manual_inspect)]` on by default
help: try
   |
45 ~         self.inner.write(bs).await.inspect(|v| {
46 |             self.written_size
47 ~                 .fetch_add(size as i64, std::sync::atomic::Ordering::Relaxed);
   |

and some other clippy error with life cycle

@Lordworms
Copy link
Author

fix ci

What problem are we fixing?

I bumped the version of both datafsuion and arrow, and fixed related ci

Cargo.toml Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think there's no need to update all other versions except datafusion.

Copy link
Member

Choose a reason for hiding this comment

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

Can only update Cargo.lock, without updating Cargo.toml

Copy link
Author

Choose a reason for hiding this comment

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

we also need to upgrade the nightlty build version in rust-toolchain.toml

thrift = "0.17.0"
tokio = { version = "1.36", default-features = false }
thrift = "0.17.0"
tokio = { version = "1.43.0", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Hi, it's better to avoid updating unrelated crates' versions unless we need to use their new APIs.

Our Cargo.toml maintains the minimal version we are using.

Copy link
Author

Choose a reason for hiding this comment

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

we need to do cargo generate-lockfile with checking minimal version, so either upgrade the tokio version or remove minimal version check.

@@ -16,5 +16,5 @@
# under the License.

[toolchain]
channel = "nightly-2024-06-10"
channel = "nightly-2024-10-17"
Copy link
Member

Choose a reason for hiding this comment

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

I believe those changes are not related. datafusion is only used in iceberg-datafusion,.

Copy link
Author

Choose a reason for hiding this comment

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

I understand, but there is a python binding ci job which should check clippy, so either we upgrade the toolchain version or ignore clippy check for datafusion integration

@Xuanwo
Copy link
Member

Xuanwo commented Mar 28, 2025

Hi, we have updated our MSRV policy and bumped some dep versions. Do we still need this?

@Xuanwo
Copy link
Member

Xuanwo commented Apr 9, 2025

Hi, thanks @Lordworms for your work on this. The issue we aimed to resolve has been fixed, so let's go ahead and close it. Feel free to open a new issue if you believe there are still any problems.

@Xuanwo Xuanwo closed this Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants