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: simplify MapApi #13063

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

drmingdrmer
Copy link
Member

@drmingdrmer drmingdrmer commented Sep 27, 2023

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

Summary

refactor: simplify MapApi

  • Add trait MapKey and MapValue to define behavior of key values
    that are used in MapApi.

  • Add Ref and RefMut as container of reference to leveled data.

  • Collect get() and range() implementation into function.

The MapApi trait can not be generalized to adapt arbitrary lifetime,
such as using self instead of &self: MapApiRO { fn get(self, key) }.
Because there is a known limitation with rust GAT and hihger ranked
lifetime: See: rust-lang/rust#114046

error: implementation of `MapApiRO` is not general enough
  --> src/meta/raft-store/src/sm_v002/sm_v002.rs:80:74
   |
80 |       async fn get_kv(&self, key: &str) -> Result<GetKVReply, Self::Error> {
   |  __________________________________________________________________________^
81 | |         let got = self.sm.get_kv(key).await;
82 | |
83 | |         let local_now_ms = SeqV::<()>::now_ms();
84 | |         let got = Self::non_expired(got, local_now_ms);
85 | |         Ok(got)
86 | |     }
   | |_____^ implementation of `MapApiRO` is not general enough
   |
   = note: `MapApiRO<'1, std::string::String>` would have to be implemented for the type `&'0 LevelData`, for any two lifetimes `'0` and `'1`...
   = note: ...but `MapApiRO<'2, std::string::String>` is actually implemented for the type `&'2 LevelData`, for some specific lifetime `'2`

Changelog

Related Issues


This change is Reviewable

@vercel
Copy link

vercel bot commented Sep 27, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
databend ⬜️ Ignored (Inspect) Visit Preview Sep 29, 2023 10:16am

@drmingdrmer drmingdrmer changed the title add dup impl to make it pass compilation refactor: support multi type leveled data Sep 27, 2023
@databendlabs databendlabs deleted a comment from databend-bot Sep 29, 2023
- Add trait `MapKey` and `MapValue` to define behavior of key values
  that are used in `MapApi`.

- Add Ref and RefMut as container of reference to leveled data.

- Collect get() and range() implementation into function.

The MapApi trait can not be generalized to adapt arbitrary lifetime,
such as using `self` instead of `&self`: `MapApiRO { fn get(self, key) }`.
Because there is a known limitation with rust GAT and hihger ranked
lifetime: See: rust-lang/rust#114046

```
error: implementation of `MapApiRO` is not general enough
  --> src/meta/raft-store/src/sm_v002/sm_v002.rs:80:74
   |
80 |       async fn get_kv(&self, key: &str) -> Result<GetKVReply, Self::Error> {
   |  __________________________________________________________________________^
81 | |         let got = self.sm.get_kv(key).await;
82 | |
83 | |         let local_now_ms = SeqV::<()>::now_ms();
84 | |         let got = Self::non_expired(got, local_now_ms);
85 | |         Ok(got)
86 | |     }
   | |_____^ implementation of `MapApiRO` is not general enough
   |
   = note: `MapApiRO<'1, std::string::String>` would have to be implemented for the type `&'0 LevelData`, for any two lifetimes `'0` and `'1`...
   = note: ...but `MapApiRO<'2, std::string::String>` is actually implemented for the type `&'2 LevelData`, for some specific lifetime `'2`
```
@drmingdrmer drmingdrmer force-pushed the 43-impl-map-api-for-ref branch from 52da1f8 to c7a53db Compare September 29, 2023 07:55
@drmingdrmer drmingdrmer changed the title refactor: support multi type leveled data ### refactor: simplify MapApi Sep 29, 2023
@drmingdrmer drmingdrmer marked this pull request as ready for review September 29, 2023 08:05
@drmingdrmer drmingdrmer changed the title ### refactor: simplify MapApi refactor: simplify MapApi Sep 29, 2023
@github-actions github-actions bot added the pr-refactor this PR changes the code base without new features or bugfix label Sep 29, 2023
@ariesdevil
Copy link
Contributor

I read a blog the other day where the author had written a library that claimed to workaround lifetime problems, I'm not sure if it's useful or not.
The blog: https://sabrinajewson.org/blog/the-better-alternative-to-lifetime-gats
The crate: https://docs.rs/nougat/latest/nougat/

@drmingdrmer
Copy link
Member Author

I read a blog the other day where the author had written a library that claimed to workaround lifetime problems, I'm not sure if it's useful or not. The blog: https://sabrinajewson.org/blog/the-better-alternative-to-lifetime-gats The crate: https://docs.rs/nougat/latest/nougat/

I sincerely appreciate your assistance!

Copy link
Member

@BohuTANG BohuTANG left a comment

Choose a reason for hiding this comment

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

Happy Mid-Autumn Day!

@BohuTANG BohuTANG merged commit c628111 into databendlabs:main Sep 29, 2023
58 checks passed
@drmingdrmer drmingdrmer deleted the 43-impl-map-api-for-ref branch September 29, 2023 14:26
@drmingdrmer
Copy link
Member Author

Happy Mid-Autumn Day!

Happy National Day!

@drmingdrmer
Copy link
Member Author

I read a blog the other day where the author had written a library that claimed to workaround lifetime problems, I'm not sure if it's useful or not. The blog: https://sabrinajewson.org/blog/the-better-alternative-to-lifetime-gats The crate: https://docs.rs/nougat/latest/nougat/

Quite good an article. It explains in detail the causes of the issue I have had, and provides a workaround solution.

The crate nougat building upon this workaround does not address my issue, though, because of the limitation of it:

  • Only lifetime GATs are supported (no type Assoc nor type Assoc<const …>).

andylokandy pushed a commit to andylokandy/databend that referenced this pull request Nov 27, 2023
* refactor: simplify MapApi

- Add trait `MapKey` and `MapValue` to define behavior of key values
  that are used in `MapApi`.

- Add Ref and RefMut as container of reference to leveled data.

- Collect get() and range() implementation into function.

The MapApi trait can not be generalized to adapt arbitrary lifetime,
such as using `self` instead of `&self`: `MapApiRO { fn get(self, key) }`.
Because there is a known limitation with rust GAT and hihger ranked
lifetime: See: rust-lang/rust#114046

```
error: implementation of `MapApiRO` is not general enough
  --> src/meta/raft-store/src/sm_v002/sm_v002.rs:80:74
   |
80 |       async fn get_kv(&self, key: &str) -> Result<GetKVReply, Self::Error> {
   |  __________________________________________________________________________^
81 | |         let got = self.sm.get_kv(key).await;
82 | |
83 | |         let local_now_ms = SeqV::<()>::now_ms();
84 | |         let got = Self::non_expired(got, local_now_ms);
85 | |         Ok(got)
86 | |     }
   | |_____^ implementation of `MapApiRO` is not general enough
   |
   = note: `MapApiRO<'1, std::string::String>` would have to be implemented for the type `&'0 LevelData`, for any two lifetimes `'0` and `'1`...
   = note: ...but `MapApiRO<'2, std::string::String>` is actually implemented for the type `&'2 LevelData`, for some specific lifetime `'2`
```

* refactor: move Ref and RefMut to separate files

* refactor: rename is_not_found() to not_found()

As @ariesdevil kindly suggested.

* refactor: simplify map api names: LevelData to Level

* chore: fix clippy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants