Add tests for sort-by

[dgr]

Closes #493

[jeaye]

Thanks, Dave!

[jeaye]

Are these testing any unique functionality that the compare + reverse compare cases don't cover? Looks to me like over-testing which we can remove.

[dgr]

Could probably cut them out. In a sense, they are testing the same thing. It's a fuzzy line between testing sort-by and the key-fn and compare functions themselves. That said (sort-by :kw > ...) is going to be pretty common. The reverse comparison should be equivalent to that if > is implemented correctly. Happy to cut all of that stuff out if you think it's redundant. I always try to be a little bit "belt-and-suspenders" when I write tests, but not overly so.

[jeaye]

@E-A-Griffin Do you have a working CLR setup to see what's up with these failures before we ping David?

[dgr]

I suspect the issue with the CLR tests is that = doesn't compare the seq and the return value from sort-by as equal, maybe because of a typing issue? I can't check CLR on my local Mac Air M1 as of now. Clojure returns a clojure.lang.ArraySeq from sort-by.

[E-A-Griffin]

Yeah I can check tonight

[dgr]

I just looked at things more closely. It's not related to comparison. For whatever reason, sort-by in CLR doesn't work with < or >, but seems to work with compare. @jeaye , see, those tests weren't redundant after all.

[E-A-Griffin]

Tested locally and reproduced the same results

[E-A-Griffin]

ClojureCLR source:

(defn sort-by
  "Returns a sorted sequence of the items in coll, where the sort
  order is determined by comparing (keyfn item).  If no comparator is
  supplied, uses compare.  comparator must implement
  java.util.Comparator.  Guaranteed to be stable: equal elements will
  not be reordered.  If coll is a Java array, it will be modified.  To
  avoid this, sort a copy of the array."
  {:added "1.0"
   :static true}
  ([keyfn coll]
   (sort-by keyfn compare coll))
  ([keyfn comp coll] 
   (sort (fn [x y] (comp (keyfn x) (keyfn y))) coll)))

Clojure

(defn sort-by
 "Returns a sorted sequence of the items in coll, where the sort
 order is determined by comparing (keyfn item).  If no comparator is
 supplied, uses compare.  comparator must implement
 java.util.Comparator.  Guaranteed to be stable: equal elements will
 not be reordered.  If coll is a Java array, it will be modified.  To
 avoid this, sort a copy of the array."
 {:added "1.0"
  :static true}
 ([keyfn coll]
  (sort-by keyfn compare coll))
 ([keyfn ^java.util.Comparator comp coll]
  (sort (fn [x y] (. comp (compare (keyfn x) (keyfn y)))) coll)))

[E-A-Griffin]

Need to verify but I think the issue is sort expects a comparator function that returns an integer value (C-style here similar to strcmp iirc) but the ClojureCLR instance is returning true/false

hence

user=> (sort-by :a #(if (> %1 %2) 1 (if (< %1 %2) -1 0)) simple-vec-maps)
({:a 1, :b 10} {:a 2, :b 9} {:a 3, :b 8} {:a 4, :b 7})

working as expected and also hence @dgr 's point about compare working

[jeaye]

Nice digging, folks. If this is looking like a bug, we can ping David to loop him in.

[dgr]

Note that in those failing tests, I'm passing < and > as the comparator. Those are predicates that return true and false, but they also both implement java.util.Comparator (so do all the comparison predicates like =, <=, >=, etc.). This allows them also to be used as comparators in sort and sort-by. I suspect the problem is located there, somehow.

[jeaye]

@dmiller for vis. This looks like it could be either a bug in Clojure CLR or at least a key difference of which we should be aware.

@dgr Your first PR back and you've already done it. I've missed you. 😁

[dmiller]

It's a bug. Dates back to 16 years ago.
no tests to detect it until now -- thanks.
Back then, I apparently did not figure out how to do sort-by in the same way as in Java. But, really, it can. In ClojureJVM, clojure.lang.AFunction implements java.util.Comparator, which is why < works. In ClojureCLR, AFunction implements System.IComparable. I just have to change the type hint and switch the call from .compare to .Compare. I did a quick test

(defn sb
  ([keyfn coll]
   (sb keyfn compare coll))
  ([keyfn ^IComparable comp coll]
   (sort (fn [x y] (. comp (Compare (keyfn x) (keyfn y)))) coll)))

resulting in

user=> (sb :a < simple-vec-maps)
({:a 1, :b 10} {:a 1, :b 10} {:a 2, :b 9} {:a 3, :b 8})

I think I'll be putting out another alpha this week, so I can drop this in there.

[jeaye]

Thank you, David! That's great to hear.

[E-A-Griffin]

@jeaye could you manually retrigger tests for this? I want to see if my merged in PR fixes some or all of the issues here

[jeaye]

@jeaye could you manually retrigger tests for this? I want to see if my merged in PR fixes some or all of the issues here

Dave will need to update the branch first.

[dgr]

You just want me to add a trivial change of some sort?

[E-A-Griffin]

Yep

[dgr]

Tweaked. CI is running...

[dgr]

Looks like CI was successful.

[jeaye]

Thank you!