Support sort-by maps and boolean comparison fns. (#709)

Hi,

could you please review update to support sorting maps with `sort-by`,
and also accepting boolean comparator fns in the same. It partly
addresses #708.

Basically, maps were delegating sorting to the immutables libraries,
which returns the keys of the maps when sorting; this patch converts
them into a sequence of key/value pairs, which is what Clojure uses to
return the result.

Boolean comparators were not working and are now converted to 3 way
comparator using the new `_fn_to_comparator` fn.

Since now the sorting is done on key/value vector pairs, it came to my
attention that the `vector.__lt__` which the sorting is delegating to,
was not working as expected, and thus updated to work according to how
Clojure thinks about vector comparison.

I've added test for all of the above.

I haven't touched `sort` much yet, wanted first to discuss this patch
first.

Thanks

---------

Co-authored-by: ikappaki <[email protected]>

Author: ikappaki

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  * Fix issue with `case` evaluating all of its clauses expressions (#699).
  * Fix issue with relative paths dropping their first character on MS-Windows (#703).
  * Fix incompatibility with `(str nil)` returning "nil" (#706).
+ * Fix `sort-by` support for maps and boolean comparator fns (#709).
 
 ## [v0.1.0a2]
 ### Added

src/basilisp/lang/runtime.py

@@ -6,6 +6,7 @@
 import itertools
 import logging
 import math
+import numbers
 import re
 import sys
 import threading
@@ -1375,39 +1376,64 @@ def _compare_sets(x: IPersistentSet, y) -> int:
 def sort(coll, f=None) -> Optional[ISeq]:
     """Return a sorted sequence of the elements in coll. If a comparator
     function f is provided, compare elements in coll using f."""
+    if isinstance(coll, IPersistentMap):
+        coll = lseq.to_seq(coll)
     return lseq.sequence(sorted(coll, key=Maybe(f).map(functools.cmp_to_key).value))
 
 
-def sort_by(keyfn, coll, cmp=None) -> Optional[ISeq]:
-    """Return a sorted sequence of the elements in coll. If a comparator
-    function f is provided, compare elements in coll using f."""
-    if cmp is not None:
+def _fn_to_comparator(f):
+    """Coerce F comparator fn to a 3 way comparator fn."""
+
+    if f == compare:  # pylint: disable=comparison-with-callable
+        return f
 
-        class key:
-            __slots__ = ("obj",)
+    def cmp(x, y):
+        r = f(x, y)
+        if isinstance(r, numbers.Number) and not isinstance(r, bool):
+            return r
+        elif r:
+            return -1
+        elif f(y, x):
+            return 1
+        else:
+            return 0
 
-            def __init__(self, obj):
-                self.obj = obj
+    return cmp
 
-            def __lt__(self, other):
-                return cmp(keyfn(self.obj), keyfn(other.obj)) < 0
 
-            def __gt__(self, other):
-                return cmp(keyfn(self.obj), keyfn(other.obj)) > 0
+def sort_by(keyfn, coll, cmp=compare) -> Optional[ISeq]:
+    """Return a sorted sequence of the elements in coll. If a
+    comparator function cmp is provided, compare elements in coll
+    using cmp or use the `compare` fn if not.
 
-            def __eq__(self, other):
-                return cmp(keyfn(self.obj), keyfn(other.obj)) == 0
+    The comparator fn can be either a boolean or 3-way comparison fn."""
+    if isinstance(coll, IPersistentMap):
+        coll = lseq.to_seq(coll)
 
-            def __le__(self, other):
-                return cmp(keyfn(self.obj), keyfn(other.obj)) <= 0
+    comparator = _fn_to_comparator(cmp)
 
-            def __ge__(self, other):
-                return cmp(keyfn(self.obj), keyfn(other.obj)) >= 0
+    class key:
+        __slots__ = ("obj",)
 
-            __hash__ = None  # type: ignore
+        def __init__(self, obj):
+            self.obj = obj
 
-    else:
-        key = keyfn  # type: ignore
+        def __lt__(self, other):
+            return comparator(keyfn(self.obj), keyfn(other.obj)) < 0
+
+        def __gt__(self, other):
+            return comparator(keyfn(self.obj), keyfn(other.obj)) > 0
+
+        def __eq__(self, other):
+            return comparator(keyfn(self.obj), keyfn(other.obj)) == 0
+
+        def __le__(self, other):
+            return comparator(keyfn(self.obj), keyfn(other.obj)) <= 0
+
+        def __ge__(self, other):
+            return comparator(keyfn(self.obj), keyfn(other.obj)) >= 0
+
+        __hash__ = None  # type: ignore
 
     return lseq.sequence(sorted(coll, key=key))
 

src/basilisp/lang/vector.py

@@ -119,13 +119,27 @@ def __len__(self):
         return len(self._inner)
 
     def __lt__(self, other):
+        """Return true if the `self` vector is shorter than the
+        `other` vector, or the first unequal element in `self` when
+        iterating from left to right is less than the corresponding
+        `other` element.
+
+        This is to support the comparing and sorting operations of
+        vectors in Clojure."""
+
         if other is None:  # pragma: no cover
             return False
         if not isinstance(other, PersistentVector):
             return NotImplemented
         if len(self) != len(other):
             return len(self) < len(other)
-        return any(x < y for x, y in zip(self, other))
+
+        for x, y in zip(self, other):
+            if x < y:
+                return True
+            elif y < x:
+                return False
+        return False
 
     def _lrepr(self, **kwargs) -> str:
         return _seq_lrepr(self._inner, "[", "]", meta=self._meta, **kwargs)

tests/basilisp/test_core_fns.lpy

@@ -200,6 +200,7 @@
 
       0  [0 1 2] [0 1 2]
       -1 [0 1 2] [1 1 2]
+      1  [2 55] [1 99]
       1  [1 1 2] [0 1 2]))
 
   (testing "un-comparables"
@@ -979,7 +980,42 @@
     (let [cmp (fn [v1 v2] (- v2 v1))]
       (is (= '() (sort-by count cmp [])))
       (is (= '([1 2 3] [5 5] [:a])
-             (sort-by count cmp [[1 2 3] [:a] [5 5]]))))))
+             (sort-by count cmp [[1 2 3] [:a] [5 5]])))))
+
+  (testing "sorting vectors"
+    (are [res v] (= res (sort-by identity v))
+      '([1] [3] [1 2]) [[1] [1 2] [3]]
+      '([1] [1 2] [1 3]) [[1 3] [1] [1 2]]
+      '([1] [0 1] [0 1]) [[0 1] [1] [0 1] ])
+
+    ;; taken from clojuredocs
+    (is (= '([1 2] [2 2] [2 3]) (sort-by first [[1 2] [2 2] [2 3]])))
+    (is (= '([2 2] [2 3] [1 2]) (sort-by first > [[1 2] [2 2] [2 3]]) )))
+
+  (testing "sorting maps"
+    (are [res v] (= res (sort-by identity v))
+      '([:3 18] [:5 28] [:9 23]) {:9 23 :3 18 :5 28})
+
+    ;; taken from clojuredocs
+    (is (= '({:rank 1} {:rank 2} {:rank 3}) (sort-by :rank [{:rank 2} {:rank 3} {:rank 1}])))
+    (let [x [{:foo 2 :bar 11}
+             {:bar 99 :foo 1}
+             {:bar 55 :foo 2}
+             {:foo 1 :bar 77}]
+          order [55 77 99 11]]
+      (is (= '({:foo 1, :bar 77} {:bar 99, :foo 1} {:foo 2, :bar 11} {:bar 55, :foo 2})
+             (sort-by (juxt :foo :bar) x)))
+      (is (= '({:bar 55, :foo 2} {:foo 1, :bar 77} {:bar 99, :foo 1} {:foo 2, :bar 11})
+             (sort-by
+              #((into {} (map-indexed (fn [i e] [e i]) order)) (:bar %))
+              x))))
+    (is (= '([:foo 7] [:baz 5] [:bar 3]) (sort-by val > {:foo 7, :bar 3, :baz 5})))
+    (is (= '({:value 1, :label "a"} {:value 2, :label "b"} {:value 3, :label "c"})
+           (sort-by :value [{:value 1 :label "a"} {:value 3 :label "c"} {:value 2 :label "b"}])))
+    (is (= '({:value 3 :label "c"} {:value 2, :label "b"} {:value 1, :label "a"})
+           (sort-by :value #(> %1 %2) [{:value 1 :label "a"} {:value 3 :label "c"} {:value 2 :label "b"}])))
+    (is (= '({:label "c"} {:value 1, :label "a"} {:value 2, :label "b"})
+           (sort-by :value [{:value 1 :label "a"} {:label "c"} {:value 2 :label "b"}])))))
 
 (deftest zipmap-test
   (are [x y z] (= x (zipmap y z))

tests/basilisp/vector_test.py

@@ -162,6 +162,38 @@ def test_vector_with_meta():
     assert vec3.meta == lmap.m(tag=keyword("macro"))
 
 
+@pytest.mark.parametrize(
+    "result,v1,v2",
+    [
+        (False, vec.v(), vec.v()),
+        (False, vec.v(1), vec.v(1)),
+        (False, vec.v(1, 2), vec.v(1, 2)),
+        (True, vec.v(1, 2), vec.v(1, 3)),
+        (False, vec.v(1, 3), vec.v(1, 2)),
+        (True, vec.v(1, 2), vec.v(1, 2, 3)),
+        (False, vec.v(1, 2, 3), vec.v(1, 2)),
+    ],
+)
+def test_vector_less_than(result, v1, v2):
+    assert result == (v1 < v2)
+
+
+@pytest.mark.parametrize(
+    "result,v1,v2",
+    [
+        (False, vec.v(), vec.v()),
+        (False, vec.v(1), vec.v(1)),
+        (False, vec.v(1, 2), vec.v(1, 2)),
+        (False, vec.v(1, 2), vec.v(1, 3)),
+        (True, vec.v(1, 3), vec.v(1, 2)),
+        (False, vec.v(1, 2), vec.v(1, 2, 3)),
+        (True, vec.v(1, 2, 3), vec.v(1, 2)),
+    ],
+)
+def test_vector_greater_than(result, v1, v2):
+    assert result == (v1 > v2)
+
+
 @pytest.mark.parametrize(
     "o",
     [