-
Notifications
You must be signed in to change notification settings - Fork 482
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
Add custom comparators #1182
base: master
Are you sure you want to change the base?
Add custom comparators #1182
Conversation
impl<'a> Equivalator<Foo> for FooComparator { | ||
fn equivalent(&self, lhs: &Foo, rhs: &Foo) -> bool { | ||
lhs == rhs | ||
} | ||
} | ||
|
||
impl<'a> Equivalator<Foo, FooRef<'a>> for FooComparator { | ||
fn equivalent(&self, foo: &Foo, key: &FooRef<'a>) -> bool { | ||
let a = u64::from_be_bytes(key.data[..8].try_into().unwrap()); | ||
let b = u32::from_be_bytes(key.data[8..].try_into().unwrap()); | ||
a == self.a && b == self.b | ||
a == foo.a && b == foo.b | ||
} | ||
} | ||
|
||
impl<'a> Comparable<FooRef<'a>> for Foo { | ||
fn compare(&self, key: &FooRef<'a>) -> std::cmp::Ordering { | ||
impl<'a> Comparator<Foo> for FooComparator { | ||
fn compare(&self, lhs: &Foo, rhs: &Foo) -> std::cmp::Ordering { | ||
Ord::cmp(lhs, rhs) | ||
} | ||
} | ||
|
||
impl<'a> Comparator<Foo, FooRef<'a>> for FooComparator { | ||
fn compare(&self, foo: &Foo, key: &FooRef<'a>) -> std::cmp::Ordering { | ||
let a = u64::from_be_bytes(key.data[..8].try_into().unwrap()); | ||
let b = u32::from_be_bytes(key.data[8..].try_into().unwrap()); | ||
Foo { a, b }.cmp(self) | ||
Foo { a, b }.cmp(foo) | ||
} | ||
} | ||
|
||
let s = SkipList::new(epoch::default_collector().clone()); | ||
let s = SkipList::with_comparator(epoch::default_collector().clone(), FooComparator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like an unfortunate increase in complexity for users who want to make stateless comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's about a dozen lines of new boilerplate. Regardless, it would be very easy to support Equivalent
/Comparable
on the default comparator. Rename OrdComparator
to BasicComparator
and define it like this:
struct BasicComparator;
impl<K: ?Sized, Q: ?Sized> Equivalator<K, Q> for BasicComparator
where
K: Equivalent<Q>,
{
#[inline]
fn equivalent(&self, lhs: &K, rhs: &Q) -> bool {
<K as Equivalent<Q>>::equivalent(lhs, rhs)
}
}
impl<K: ?Sized, Q: ?Sized> Comparator<K, Q> for BasicComparator
where
K: Comparable<Q>,
{
#[inline]
fn compare(&self, lhs: &K, rhs: &Q) -> Ordering {
<K as Comparable<Q>>::compare(lhs, rhs)
}
}
In some sense this is the best of both worlds. The only downside would be that there are now two sets of traits (Equivalent
/Comparable
, Equivalator
/Comparator
), which might be mildly confusing at first sight.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, two sets of traits are reasonable. Actually, it is just one set of traits for stateless comparison, because, for stateless comparison, users just use the BasicComparator
(the default one) and only need to implement Equivalent
and Comparable
. They do not need to care about Equivalentor
and Comparator
.
cc @al8n |
Closes #1180.
The diff is large due to having to add a type parameter
C
and update constraints in numerous places, but the logic changes are not large.This trait adds the
Comparator
andEquivalator
traits, which generalize theComparable
andEquivalent
traits by taking aself
parameter in addition to the two keys to compare, making it possible to change the comparison function used without changing the key type.Summary
trait Comparator<L, R>
andtrait Equivalator<L, R>
.struct OrdComparator
, which is a zero-sized type that implementsComparator
usingBorrow
andOrd
.C
toSkipList
,SkipMap
, andSkipSet
.C
is defaulted toOrdComparator
. This preserves the existing behavior of functions likeget()
,insert()
, etc.SkipList::with_comparator()
,SkipSet::with_comparator()
, andSkipMap::with_comparator()
constructors.SkipList::new()
,SkipMap::new()
, andSkipSet::new()
always useC = OrdComparator
. It would be nice if it worked for anyC
that implementsDefault
, but that would break type inference. This is exactly the reasonstd::collections::HashMap::new()
always usesS = RandomState
for its defaulted parameter even ifS
implementsDefault
.Box<dyn Fn(&[u8], &[u8]) -> Ordering>
as the comparator.Hashing
Because the goal of these traits is to support totally different comparison operations from the default for the underlying key type,
Equivalator
explicitly does not require two equivalent keys to have the same hash. If someone, someday wants to supportEquivalator
on their hash map, they will have to generalize theHash
trait as well.Safety
My one minor question is whether
unsafe impl<Q, R, K, V, C> Send for RefRange<'_, Q, R, K, V, C>
should requireC: Sync
. I don't think so because it doesn't requireK
orV
to be sync, but it was enough to make me second-guess myself.