Generic Matchers

[connortsui20]

Summary

This PR pakes the Matcher pattern we have been using generic, and replaces the existing duplicated traits. The changes can be roughly seen in this order by the 2 commits.

We want this because we will be adding matchers for the extension scalars and arrays in the future, and having 4 duplicated traits that are exactly the same except for 1 parameter is a bit much.

---

The old trait(s) was this before:

pub trait Matcher {
    type Match<'a>;

    /// Check if the given array matches this matcher type
    //                 vvvvvvvvv This is hardcoded!
    fn matches(array: &dyn Array) -> bool {
        Self::try_match(array).is_some()
    }

    /// Try to match the given array, returning the matched view type if successful.
    //                          vvvvvvvvv This is hardcoded!
    fn try_match<'a>(array: &'a dyn Array) -> Option<Self::Match<'a>>;
}

The new trait(s) is now:

/// A super trait that allows us to define a generic associated type on `Matcher`.
///
/// We need to do this in this manner because of rust needs to keep forward compatibility.
/// See [this](https://github.com/rust-lang/rust/issues/87479) GitHub issue for more information.
pub trait MatcherType<Over: ?Sized> {
    /// The matched view type.
    type Match<'a>;
}

/// A trait for matching types.
pub trait Matcher<Over: ?Sized>: MatcherType<Over> {
    /// Check if the given item matches this matcher.
    fn matches(item: &Over) -> bool {
        Self::try_match(item).is_some()
    }

    /// Try to match the given item, returning the matched type if successful.
    fn try_match<'a>(item: &'a Over) -> Option<Self::Match<'a>>;
}

Why 2 traits?

Note that the issue in rust-lang/rust#87479 is the reason we need to split this out into 2 traits. This meant changing some fully qualified paths, but that is just a cosmetic change.

Why generic MatcherType<Over: ?Sized>?

Also, you might wonder why the MatcherType also has a generic parameter. This is because we have both a impl<V: VTable> MatcherType<dyn Array> for V and a impl<V: ExtVTable> MatcherType<ExtDTypeRef> for V, and without that generic parameter the trait solver cannot distinguish that VTable is mutually exclusive from ExtVTable.

Unresolved Questions

It might be good to also type alias the Matcher<dyn Array> to an ArrayMatcher? Not sure if that's any better.

API Changes

Technically there are API changes here but it is mostly related to extension types which nobody should be using right now anyways.

Testing

Since this is a generics change there is not logical difference.

[AdamGS]

nit - can we keep generic bounds as acronyms/obviously not words, I personally find it confusing but also I've never really seen a rust codebase do that

[connortsui20]

Should it just be T?

[AdamGS]

or O?

[gatesn]

What is MatcherType?

(Want to block until we can chat about this)

[codspeed-hq]

Merging this PR will improve performance by 43.1%

⚡ 1 improved benchmark
✅ 953 untouched benchmarks
⏩ 1466 skipped benchmarks¹

Performance Changes

	Mode	Benchmark	BASE	HEAD	Efficiency
⚡	Simulation	chunked_bool_canonical_into[(1000, 10)]	68.2 µs	47.7 µs	+43.1%

---

_{Comparing ct/generic-matcher (dab9b65) with develop (7bc4d0b)}

Footnotes

1. 1466 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[connortsui20]

MatcherType is just a supertrait. To be honest, I don't fully understand why we need to do this but we get a compiler error if I add a generic. See rust-lang/rust#87479 for more details.

[gatesn]

I'm not sure this is any better than separate matcher traits tbh 🤷