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

Custom key comparison functions #90

Closed
wants to merge 7 commits into from
Closed

Conversation

Kerollmops
Copy link
Member

@Kerollmops Kerollmops commented Jan 7, 2021

This PR adds some new method for database opening to let the user specify a custom key comparison function if necessary and fixes #86.

  • Expose an API to create/open databases with a custom key comparison function.
  • Ensure that a database correctly uses a comparison function if it has been open with one.
  • Expose a safe Rust API for the user (instead of an extern "C" one).
  • Move this CustomKeyCmp trait into the heed-trait crate.
  • Ask for a typed function (using the user-defined key codec) instead of a function that takes byte slices.
  • Change back the CustomKeyCmp trait inputs to be byte slices again, this way we avoid the decoding overheads.
  • Make it compile with MDBX but don't let users use that feature as custom key comparator are deprecated.

Expose a Rust API instead of the LMDB required function signature

The current exposed API for the custom key comparison function is an ugly extern "C" function, I would like to let the user give a real Rust function instead. It is ugly in the sense that I don't want to expose the MDB_val type (a struct with start and length fields) and therefore the from_val function (transform an MDB_val into a Rust slice).

// What the API user must define itself, we give that to LMDB directly.
extern "C" fn my_custom_key_cmp(a: *const MDB_val, b: *const MDB_val) -> i32;

// What I would like to ask the user to give and wrap.
fn my_custom_key_cmp(a: &[u8], b: &[u8]) -> Ordering;

// The wrapper that could be used to call the user defined function,
// transforming a C call into a Rust call, invisible to the API user.
extern "C" fn wrapper(a: *const MDB_val, b: *const MDB_val) -> i32 {
    // transform a and b into Rust slices here using `from_val`.
    // PROBLEM how can I pass the user function here? There is no easy way.
    match (user_cmp_fn)(a, b) {
        Ordering::Less => -1,
        Ordering::Equal => 0,
        Ordering::Greater => 1,
    }
}

As it is not quite possible to pass the user-defined function to a wrapper function with the LMDB required signature, I thought that we could maybe be able to "generate" a function with the required signature that is monomorphized to call the user-defined function (no hidden parameter), but is it possible to do that, I am not sure, I tried by using a closure and a Box::into_raw call, but it didn't work well (dyn Fn can't be cast into extern "C" fn).

Ask for a typed key comparison function instead of a function that takes byte slices

The current exposed API asks for a Rust function that takes two bytes slices as arguments and returns an Ordering, it is a good enough solution but it would maybe be better to ask for a function that takes decoded types instead, using the database key codec.

// The trait currently asked the user that is used to compare two keys.
pub trait CustomKeyCmp {
    fn compare(a: &[u8], b: &[u8]) -> Ordering;
}

// The trait that I would like to expose to the user.
pub trait CustomKeyCmp<'t> {
    type Key: 't;
    fn compare(a: Self::Key, b: Self::Key) -> Ordering;
}

@Kerollmops Kerollmops added the enhancement New feature or request label Jan 7, 2021
@valpackett
Copy link

Perfect solution: ask LMDB to change the signature to add a void *user_data that would be passed to the comparator.

Hack: pass the rust function via a thread_local? :D

Keep in mind: https://matklad.github.io/2020/10/03/fast-thread-locals-in-rust.html

@valpackett
Copy link

extern "C" fn wrapper<C: CustomKeyCmp>

Yeah, for just static functions (not closures) this is perfect, and if users want to capture any state, they have to do global passing themselves

@Kerollmops Kerollmops changed the title Custom comparison functions Custom key comparison functions Jan 8, 2021
@Kerollmops
Copy link
Member Author

Thank you for the help @myfreeweb,

Indeed it is not easy to accept a user-defined Rust closure as the custom key comparison function. A lot of people helped me on Reddit and some of them were proposing this solution (using a trait as a generic parameter to an extern "C" function) therefore generating the appropriate function for LMDB.

Yeah, for just static functions (not closures) this is perfect, and if users want to capture any state, they have to do global passing themselves

And yes, this can be a good enough solution for those who really need a Rust closure with a context or something.

heed/src/env.rs Outdated Show resolved Hide resolved
@Kerollmops
Copy link
Member Author

The complexity is more there than what I thought, introducing a custom key comparison function to the database (that is in fact based on the LMDB one) introduces a lot of little behavior like the fact that we can't just advance and retreat a key by only using the bytes of it as it is not necessarily the way it works (think about integer in little-endian).

So I am closing this now as I don't want to take more time on that problem for now.

@Kerollmops Kerollmops closed this Jan 12, 2021
@Kerollmops Kerollmops deleted the custom-cmp-function branch January 12, 2021 13:08
@Kerollmops Kerollmops restored the custom-cmp-function branch January 12, 2021 13:08
@curquiza curquiza deleted the custom-cmp-function branch April 21, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for custom key comparison function
2 participants