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

refactor(ci): bump rust version #9540

Merged
merged 16 commits into from
Jan 11, 2023

Conversation

ClSlaid
Copy link
Contributor

@ClSlaid ClSlaid commented Jan 9, 2023

Signed-off-by: 蔡略 [email protected]

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

bump rust toolchain to rust nightly-2022-12-15

Change log

  1. update CI toolchain to nightly-2022-12-15
  2. update rust toolchain to nightly-2022-12-15
  3. allows clippy::unlined_format_args
  4. update stateless-test/10_drivers/10_0001_python_sqlalchemy_driver for sqlalchemy 2.0 compatibilities, this makes this PR a refactor
  5. remove unnecessary lifetime annotations
  6. fix some higher-ranked lifetime problem introduced in the new toolchain, thanks to @Xuanwo
  7. some bonus changes, like changing (b'0'..b'9').contains(k) to k.is_ascii_digit()
  8. make cargo-machete ignore match-template in storages-common-index

Closes #issue

bump rust toolchain to rust nightly-2022-12-15

Signed-off-by: 蔡略 <[email protected]>
@ClSlaid ClSlaid requested a review from Xuanwo January 9, 2023 13:09
@vercel
Copy link

vercel bot commented Jan 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
databend ⬜️ Ignored (Inspect) Jan 11, 2023 at 7:26AM (UTC)

@mergify mergify bot added the pr-chore this PR only has small changes that no need to record, like coding styles. label Jan 9, 2023
@ClSlaid ClSlaid requested a review from PsiACE January 9, 2023 13:11
@Xuanwo
Copy link
Member

Xuanwo commented Jan 9, 2023

Please follow https://databend.rs/doc/faq/routine-maintenance to bump rust toolchain.

rust-toolchain.toml Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Jan 10, 2023

This PR is blocked by rust-lang/rust#64552

I tried some workaround but failing with:

error: implementation of `std::marker::Send` is not general enough
   --> src/query/storages/fuse/src/operations/mutation/deletion/deletion_transform.rs:331:53
    |
331 |       async fn async_process(&mut self) -> Result<()> {
    |  _____________________________________________________^
332 | |         match std::mem::replace(&mut self.state, State::None) {
333 | |             State::ReadSegments => {
334 | |                 let ctx = self.ctx.clone();
...   |
368 | |         Ok(())
369 | |     }
    | |_____^ implementation of `std::marker::Send` is not general enough
    |
    = note: `std::marker::Send` would have to be implemented for the type `&'0 [storages_common_table_meta::meta::Location]`, for any lifetime `'0`...
    = note: ...but `std::marker::Send` is actually implemented for the type `&'1 [storages_common_table_meta::meta::Location]`, for some specific lifetime `'1`

Seems we need to refactor SegmentsIO::read_segments to make it pass:

#[tracing::instrument(level = "debug", skip_all)]
pub async fn read_segments(
    &self,
    segment_locations: &[Location],
) -> Result<Vec<Result<Arc<SegmentInfo>>>> {}

@Xuanwo
Copy link
Member

Xuanwo commented Jan 10, 2023

Now I'm sure problem happens at this arm:

    async fn async_process(&mut self) -> Result<()> {
        let state = std::mem::replace(&mut self.state, State::None);

        match state {
            // State::ReadSegments => {
            //     let ctx = self.ctx.clone();
            //     let dal = self.dal.clone();
            //     let schema = self.schema.clone();
            //     let segment_locations = self.base_segments.clone();

            //     let fut = Box::pin(async move {
            //         // Read all segments information in parallel.
            //         SegmentsIO::read_segments_work_around(ctx, dal, schema, segment_locations)
            //             .await?
            //             .into_iter()
            //             .collect::<Result<Vec<_>>>()
            //     });

            //     let segments = fut.await?;

            //     self.state = State::GenerateSegments(segments);
            // }
            State::SerializedSegments {
                serialized_data,
                segments,
                summary,
            } => {
                let ctx = self.ctx.clone();
                let dal = self.dal.clone();

                Self::write_segments(ctx, dal, serialized_data).await?;

                self.state = State::Output { segments, summary };
            }
            _ => return Err(ErrorCode::Internal("It's a bug.")),
        }
}

@Xuanwo
Copy link
Member

Xuanwo commented Jan 10, 2023

More deeper, this call:

// Self::write_segments(ctx, dal, serialized_data).await?;

@ClSlaid
Copy link
Contributor Author

ClSlaid commented Jan 10, 2023

Seems we need to refactor SegmentsIO::read_segments to make it pass:

The Location now is just an alias for (String, FormatVersion), maybe changing it to a newtype style could solve it.

pub struct Location(String, FormatVersion)

But it will introduce much more refactor, so if it does solve the problem, I'll make an independent PR for the introduced refactor.

@Xuanwo
Copy link
Member

Xuanwo commented Jan 10, 2023

Ok, I got it:

pub async fn try_join_futures<Fut>(
    ctx: Arc<dyn TableContext>,
-    futures: impl IntoIterator<Item = Fut>,
+    futures: Vec<Fut>,
    thread_name: String,
) -> Result<Vec<Fut::Output>>

@Xuanwo
Copy link
Member

Xuanwo commented Jan 10, 2023

But it will introduce much more refactor, so if it does solve the problem, I'll make an independent PR for the introduced refactor.

I tried locally, and not fixed.

auto code generation has derivable `Default`s and single armed matches,
let clippy ignore it

Signed-off-by: 蔡略 <[email protected]>
engine.connect() is deprecated in sqlalchemy2.0
use `engine.begin()` instead

Signed-off-by: 蔡略 <[email protected]>
@Xuanwo
Copy link
Member

Xuanwo commented Jan 11, 2023

All warnings in hms_patched.rs can be ignored here:

https://github.com/datafuselabs/databend/blob/231a1e9f7f7ca6e4b1da740be3a8013331b93963/src/query/storages/hive/hive-meta-store/src/lib.rs#L15-L23

13465 |       ()
      |       ^^ help: remove the final `()`
      |
      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unused_unit

@Xuanwo
Copy link
Member

Xuanwo commented Jan 11, 2023

error: redundant field names in struct initialization
     --> /workspace/target/debug/build/common-hive-meta-store-d2e71787906d9704/out/hms_patched.rs:24315:72
      |
24315 |         let call_args = ThriftHiveMetastoreGetTableNamesByFilterArgs { dbname: dbname, filter: filter, max_tables: max_tables };
      |                                                                        ^^^^^^^^^^^^^^ help: replace it with: `dbname`
      |
      = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_field_names

clippy is so strict (

a crate have to import all of foreign macros it depends on
but cargo-machete is not clever enough to detect this

so those dependencies have to be ignored

Signed-off-by: 蔡略 <[email protected]>
@ClSlaid
Copy link
Contributor Author

ClSlaid commented Jan 11, 2023

clippy is so strict (

Code generator could be improved in the future, let's allow this temporarily.

@ClSlaid ClSlaid changed the title chore(toolchain): bump rust version refactor(ci): bump rust version Jan 11, 2023
@Xuanwo
Copy link
Member

Xuanwo commented Jan 11, 2023

Code generator could be improved in the future, let's allow this temporarily.

We don't need to improve this part, they are not maintained by human. Disable all lint for generated code LGTM.

@mergify mergify bot added the pr-refactor this PR changes the code base without new features or bugfix label Jan 11, 2023
1. use `char::is_ascii_digit() -> bool` instead of
   (b'0'..=b'9').contains(char)

Signed-off-by: 蔡略 <[email protected]>
@Xuanwo
Copy link
Member

Xuanwo commented Jan 11, 2023

All check passed! Thanks for the hard work of @ClSlaid

@mergify mergify bot merged commit 9bb1678 into databendlabs:main Jan 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-chore this PR only has small changes that no need to record, like coding styles. pr-refactor this PR changes the code base without new features or bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants