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

feat: add dup_sort_comparator #283

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

oXtxNt9U
Copy link

@oXtxNt9U oXtxNt9U commented Sep 24, 2024

Pull Request

What does this PR do?

Expose http://www.lmdb.tech/doc/group__internal.html#gacef4ec3dab0bbd9bc978b73c19c879ae via new dup_sort_comparator function to set a custom Comparator.

It basically works like key_comparator, except that for calling mdb_set_compare it calls mdb_set_dupsort on the database.

It can be used like this:

enum DescendingIntCmp {}

impl Comparator for DescendingIntCmp {
    fn compare(a: &[u8], b: &[u8]) -> Ordering {
        b.cmp(&a)
    }
}

//

let db = env
    .database_options()
    .types::<Str, U128<BigEndian>>()
    .flags(DatabaseFlags::DUP_SORT)
    .dup_sort_comparator::<DescendingIntCmp>()
    .create(&mut wtxn)?;

Also added an example and updated the cookbook (resolves #284).

@Kerollmops
Copy link
Member

Kerollmops commented Sep 24, 2024

Hey @oXtxNt9U 👋

They're already is a way to use custom comparison functions in a safe way in heed. You can see that in the database open options, you just have to use the Compare trait.

However, it was hard, even for me, to find that in the documentation. Would you mind adding a small guide (in the top three) to the cookbook to show people that we support it and how to use it? There already are some examples in this PR (whoops, nope).

Have a nice day and please close this PR if I was right 😊

@oXtxNt9U
Copy link
Author

oXtxNt9U commented Sep 24, 2024

@Kerollmops Thank you for pointing out the Compare trait. Upon closer inspection, it only calls mdb_set_compare but what I need is mdb_set_dupsort because only then it passes the values to the compare function instead of the keys.

Basically, what I need is something like this inside raw_open_dbi:

unsafe {
    mdb_result(ffi::mdb_dbi_open(raw_txn, name_ptr, flags, &mut dbi))?;
    if TypeId::of::<C>() != TypeId::of::<DefaultComparator>() {
        if AllDatabaseFlags::from_bits(flags)
            .is_some_and(|f| f.contains(AllDatabaseFlags::DUP_SORT))
        {
            mdb_result(ffi::mdb_set_dupsort(
                raw_txn,
                dbi,
                Some(custom_key_cmp_wrapper::<C>),
            ))?;
        } else {
            mdb_result(ffi::mdb_set_compare(
                raw_txn,
                dbi,
                Some(custom_key_cmp_wrapper::<C>),
            ))?;
        }
    }
};

However this makes it impossible to use a different compare for both keys and values since it reuses C. So I could update my PR to introduce a new type parameter CDUP which if present would hook up a custom mdb_set_dupsort.

let db = env
    .database_options()
    .types::<StorageKey, StorageValue>()
    .dup_sort_comparator::<CustomComparator>() // sets CDUP
    .flags(DatabaseFlags::DUP_SORT)
    .create(&mut wtxn)
    .expect("db");

What do you think?

@Kerollmops
Copy link
Member

So I could update my PR to introduce a new type parameter CDUP which if present would hook up a custom mdb_set_dupsort [..] What do you think?

Yeah! That could be much better. Also, not breaking APIs could be great too but may be impossible.
I also created an issue about documenting the custom comparison functions more, if you want to take a look.

@oXtxNt9U oXtxNt9U changed the title feat: expose mdb_set_dupsort feat: add dup_sort_comparator Sep 25, 2024
@oXtxNt9U
Copy link
Author

oXtxNt9U commented Sep 25, 2024

@Kerollmops Hey, I updated the PR and included examples for the cookbook too. Please let me know if anything is still missing.

@oXtxNt9U
Copy link
Author

@Kerollmops Friendly ping! 😄

@Kerollmops
Copy link
Member

Hey @oXtxNt9U 👋

Thank you for the changes. Everything looks good to me. However, it is breaking, so I would prefer to wait for some other PRs to be released first. Also, we merged a couple of PRs on the main. Would you mind rebasing, please?

Have a nice end of the week 🌹

@oXtxNt9U
Copy link
Author

@Kerollmops I rebased the PR. And of course I don't mind waiting for a new release. Thank you!

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.

Add a guide to the cookbook about custom comparator functions
2 participants