[Pitch] Add max, min and lexicographicallyPrecedes using SortComparator

Add max, min and lexicographicallyPrecedes using SortComparator

Introduction

Foundation has the method sorted(using:) to sort sequences using objects conforming to SortComparator, such as KeyPathComparator:

let people: [Person] = [...]

let peopleSortedByAge = people.sorted(using: KeyPathComparator(\.age))

This method calls sorted(by:), which uses a (Self.Element, Self.Element) throws -> Bool) closure to sort the sequence elements.

The Sequence protocol has two other methods that uses the same closure type, max(by:), min(by:) and lexicographicallyPrecedes(_:by:):

let oldestPerson = people.max(by: {
    $0.age < $1.age
}

let youngestPerson = people.min(by: {
    $0.age < $1.age
})

let isFirstGroupYounger = firstGroup.lexicographicallyPrecedes(secondGroup, by: { $0.age < $1.age })

These methods could benefit from counterparts using SortComparator as well.

Proposed Solution

Introduce max(using:), min(using:) and lexicographicallyPrecedes(_:using:) methods to use a comparator in these operations.

let oldestPerson = people.max(using: KeyPathComparator(\.age))

let youngestPerson = people.min(using: KeyPathComparator(\.age))

let isFirstGroupYounger = firstGroup.lexicographicallyPrecedes(secondGroup, using: KeyPathComparator(\.age))

Detailed Design

extension Sequence {
    /// Returns the maximum element in the sequence, using the given comparator
    /// to compare elements.
    ///
    /// - Parameters:
    ///   - comparator: the comparator to use in ordering elements
    /// - Returns: The sequence's maximum element, according to `comparator`.
    ///   If the sequence has no elements, returns `nil`.
    public func max(using comparator: some SortComparator<Element>) -> Element? {
        return self.max {
            comparator.compare($0, $1) == .orderedAscending
        }
    }
    /// Returns the minimum element in the sequence, using the given comparator as
    /// to compare elements.
    ///
    /// - Parameters:
    ///   - comparator: the comparator to use in ordering elements
    /// - Returns: The sequence's minimum element, according to `comparator`.
    ///   If the sequence has no elements, returns `nil`.
    public func min(using comparator: some SortComparator<Element>) -> Element? {
        return self.min {
            comparator.compare($0, $1) == .orderedAscending
        }
    }
    /// Returns a Boolean value indicating whether the sequence precedes another
    /// sequence in a lexicographical (dictionary) ordering, using the given
    /// comparator to compare elements.
    ///
    /// - Parameters:
    ///   - other: A sequence to compare to this sequence.
    ///   - comparator: the comparator to use in ordering elements.
    /// - Returns: `true` if this sequence precedes `other` in a dictionary
    ///   ordering as ordered by `comparator`; otherwise, `false`.
    public func lexicographicallyPrecedes(_ other: some Sequence<Element>, using comparator: some SortComparator<Element>) -> Bool {
        return self.lexicographicallyPrecedes(other) {
            comparator.compare($0, $1) == .orderedAscending
        }
    }
}
5 Likes

+1 from me! This looks like a great addition, given we already have the sorted(using:) version it seems helpful to have the max(using:)/min(using:) version to mirror those, and that'd definitely be more efficient than a .sorted(using:).first/.sorted(using:).last implementation that people might reach for.

I'm sure this is useful, but I don't think it makes any sense to add this before adding the missing closure-based overloads.

let oldestPerson = people.max(by: \.age)
let youngestPerson = people.min(by: \.age)
public extension Sequence {
  @inlinable func max<each Comparable: Swift.Comparable, Error>(
    by comparable: (Element) throws(Error) -> (repeat each Comparable)
  ) throws(Error) -> Element? {
    do {
      return try self.max {
        try (repeat each comparable($0)) < (repeat each comparable($1))
      }
    } catch {
      throw error as! Error
    }
  }

  @inlinable func min<each Comparable: Swift.Comparable, Error>(
    by comparable: (Element) throws(Error) -> (repeat each Comparable)
  ) throws(Error) -> Element? {
    do {
      return try self.min {
        try (repeat each comparable($0)) < (repeat each comparable($1))
      }
    } catch {
      throw error as! Error
    }
  }
}
<
@inlinable public func < <each Element: Comparable>(
  _ element0: (repeat each Element),
  _ element1: (repeat each Element)
) -> Bool {
  for elements in repeat (each element0, each element1) {
    if elements.0 < elements.1 { return true }
    if elements.0 > elements.1 { return false }
  }
  return false
}

The relevant sortedoverload is also missing.

Thoughts on using an any-typed parameter instead of a concrete generic?

eg

func max(using comparator: any SortComparator<Element>) -> Element?

Rather than reimplementing the various Sequence members that might take a SortComparator instead of a predicate closure, what about adding a member to SortComparator that can serve as a predicate closure? Then you'd get min(by:), max(by:), sorted(by:) (which already has an overload, I know, but still), partition(by:), and whatever other members are added in the future that compare elements.

(Members that compare for equality only can be trivially derived too.)

1 Like

I honestly just followed the same signature of sorted(using:), it sure makes the signature shorter. But I personally would actually use some SortComparator<Element> instead.

2 Likes

any as a form of type erasure has performance consequences that some or a generic parameter wouldn't have, especially if the function can be inlined.

("What are the performance consequences?" I don't remember offhand but they're real, I swearsies.)

3 Likes

Correct - for this reason we should definitely opt for a generic/some variant over an any variant. A generic function is provided with the types metadata and can access the type much more efficiently than when using an existential which requires dynamically looking up the metadata before it can use the type. some can be used for brevity here as it behaves the same as a generic constraint.

Taking a look at the APIs we have today, it looks like the other APIs that this could apply to are partition(by:) and lexicographicallyPrecedes(by:). We could add a closure property to SortComparator to allow you to pass it to various APIs, something like myCollection.max(by: KeyPathComparator(\.age).closure). However, given the number of APIs we're talking about here currently (5 total, one of which is already extended), I'd tend to lean towards overloading the APIs with SortComparator just for ease of use by developers. I'd have to go look at APIs to get some evidence to back this up, but I feel like we've done this before in Foundation where we've provided the tools directly for developers to use rather than providing the pieces and have developers do the "work" of finding the right API to connect them together (in this case, that "closure" property). But that's at least my first thought having needed to write code like this before.

3 Likes

I can’t find a partition(by:) that could be overload with SortComparator, just one that takes a closure of type (Self.Element) throws -> Bool.

Oh my mistake! Yes you're right the signature doesn't quite match because partitioning operates by categorizing individual elements rather than comparing elements to each other, so you're right this wouldn't be applicable here.

Oops! Indeed.

I've update the pitch to include lexicographicallyPrecedes and use opaque type instead generic constraint.

There are two; don't forget the other one.

What you are suggesting looks like this:

public extension Sequence {
  @inlinable func max<Comparator: SortComparator<Element>>(
    using comparators: some Sequence<Comparator>
  ) -> Element? {
    self.max { comparators.compare($0, $1) == .orderedAscending }
  }
}
people.max(using: [
  KeyPathComparator(\.age),
  .init(\.beauty, order: .reverse)
])

I do not think that is good. The precedent should be being set for variadics, not more arrays of SortComparators.

people.max(by: (\.age, <), (\.beauty, >))
public extension Sequence {
  @inlinable func max<each Comparable: Swift.Comparable, Error>(
    by: repeat (
      comparable: (Element) throws(Error) -> each Comparable,
      areInIncreasingOrder: (each Comparable, each Comparable) throws(Error) -> Bool
    )
  ) throws(Error) -> Element? {
    do {
      return try self.max {
        for (elements, areInIncreasingOrder) in repeat try (
          ((each by).comparable($0), (each by).comparable($1)), (each by).areInIncreasingOrder
        ) {
          if try areInIncreasingOrder(elements.0, elements.1) { return true }
          if try areInIncreasingOrder(elements.1, elements.0) { return false }
        }
        return false // Equality.
      }
    } catch {
      throw error as! Error
    }
  }
}
1 Like

Thanks, I missed this one.

That's certainly a different approach to achieve the same goal, but my intention with this pitch is to just expand the current usage of SortComparator to more fitting methods. I believe alternatives to this that don't use SortComparator should probably be proposed to the standard library.

1 Like