refactored discover hit sort, added tests

src/kibana/apps/discover/_hit_sort_fn.js

@@ -0,0 +1,66 @@
+define(function () {
+  return function HitSortFnFactory() {
+
+    /**
+     * Creates a sort function that will resort hits based on the value
+     * es used to sort them.
+     *
+     * background:
+     * When a hit is sorted by elasticsearch, es will write the values that it used
+     * to sort them into an array at the top level of the hit like so
+     *
+     * ```
+     * hits: {
+     *   total: x,
+     *   hits: [
+     *     {
+     *       _id: i,
+     *       _source: {},
+     *       sort: [
+     *         // all values used to sort, in the order of precidance
+     *       ]
+     *     }
+     *   ]
+     * };
+     * ```
+     *
+     * @param  {[type]} field     [description]
+     * @param  {[type]} direction [description]
+     * @return {[type]}           [description]
+     */
+    return function createHitSortFn(direction) {
+      var descending = (direction === 'desc');
+
+      return function sortHits(hitA, hitB) {
+        var bBelowa = null;
+
+        var aSorts = hitA.sort || [];
+        var bSorts = hitB.sort || [];
+
+        // walk each sort value, and compair until one is different
+        for (var i = 0; i < bSorts.length; i++) {
+          var a = aSorts[i];
+          var b = bSorts[i];
+
+          if (a == null || b > a) {
+            bBelowa = !descending;
+            break;
+          }
+
+          if (b < a) {
+            bBelowa = descending;
+            break;
+          }
+        }
+
+        if (bBelowa !== null) {
+          return bBelowa ? -1 : 1;
+        } else {
+          return 0;
+        }
+
+      };
+    };
+
+  };
+});

src/kibana/apps/discover/controllers/discover.js

@@ -52,6 +52,8 @@ define(function (require) {
     Notifier, $location, globalState, AppState, timefilter, AdhocVis, Promise, Private) {
 
     var segmentedFetch = $scope.segmentedFetch = Private(require('apps/discover/_segmented_fetch'));
+    var HitSortFn = Private(require('apps/discover/_hit_sort_fn'));
+
     var notify = new Notifier({
       location: 'Discover'
     });
@@ -222,6 +224,11 @@ define(function (require) {
           else return 'non-time';
         }());
 
+        var sortFn = null;
+        if (sortBy === 'non-time') {
+          sortFn = new HitSortFn(sort[1]);
+        }
+
         var eventComplete = notify.event('segmented fetch');
 
         return segmentedFetch.fetch({
@@ -242,29 +249,16 @@ define(function (require) {
             rows = $scope.rows = rows.concat(resp.hits.hits);
             rows.fieldCounts = counts;
 
-            if (sortBy === 'non-time') {
-              rows.sort(function (rowA, rowB) {
-                var dir = 0;
-                var a = rowA.sort[0];
-                var b = rowB.sort[0];
-
-                if (a < b) dir = 1;
-                else if (a > b) dir = -1;
-
-                if (sort[1] === 'desc') {
-                  dir = dir * -1;
-                }
-                return dir;
-              });
-
+            if (sortFn) {
+              rows.sort(sortFn);
               rows = $scope.rows = rows.slice(0, totalSize);
               counts = rows.fieldCounts = {};
             }
 
             $scope.rows.forEach(function (hit) {
-              // when we are sorting by non time field, our rows will vary too
-              // much to rely on the previous counts, so we have to do this all again.
-              if (sortBy === 'non-time' && hit._formatted) return;
+              // when we are resorting on each segment we need to rebuild the
+              // counts each time
+              if (sortFn && hit._formatted) return;
 
               hit._formatted = _.mapValues(hit._source, function (value, name) {
                 // add up the counts for each field name

test/unit/index.html

@@ -60,7 +60,7 @@
         require([
           'kibana',
           'sinon/sinon',
-          //'specs/apps/dashboard/directives/panel',
+          'specs/apps/discover/hit_sort_fn',
           'specs/directives/timepicker',
           'specs/directives/truncate',
           'specs/directives/css_truncate',

test/unit/specs/apps/discover/hit_sort_fn.js

@@ -0,0 +1,75 @@
+define(function (require) {
+  var _ = require('lodash');
+
+  require('angular').module('hitSortFunctionTests', ['kibana']);
+
+  describe('hit sort function', function () {
+    var createHitSortFn;
+
+    beforeEach(module('hitSortFunctionTests'));
+    beforeEach(inject(function (Private) {
+      createHitSortFn = Private(require('apps/discover/_hit_sort_fn'));
+    }));
+
+
+    var runSortTest = function (dir, sortOpts) {
+      var groupSize = _.random(10, 30);
+      var total = sortOpts.length * groupSize;
+
+      var hits = new Array(total);
+      sortOpts = sortOpts.map(function (opt) {
+        if (_.isArray(opt)) return opt;
+        else return [opt];
+      });
+      var sortOptLength = sortOpts.length;
+
+      for (var i = 0; i < hits.length; i++) {
+        hits[i] = {
+          _source: {},
+          sort: sortOpts[i % sortOptLength]
+        };
+      }
+
+      hits.sort(createHitSortFn(dir))
+      .forEach(function (hit, i, hits) {
+        var group = Math.floor(i / groupSize);
+        expect(hit.sort).to.eql(sortOpts[group]);
+      });
+    };
+
+
+    it('sorts a list of hits in ascending order', function () {
+      runSortTest('asc', [200, 404, 500]);
+    });
+
+    it('sorts a list of hits in descending order', function () {
+      runSortTest('desc', [10, 3, 1]);
+    });
+
+    it('breaks ties in ascending order', function () {
+      runSortTest('asc', [
+        [ 'apache',  200,  'facebook.com' ],
+        [ 'apache',  200,  'twitter.com'  ],
+        [ 'apache',  301,  'facebook.com' ],
+        [ 'apache',  301,  'twitter.com'  ],
+        [ 'nginx',   200,  'facebook.com' ],
+        [ 'nginx',   200,  'twitter.com'  ],
+        [ 'nginx',   301,  'facebook.com' ],
+        [ 'nginx',   301,  'twitter.com'  ]
+      ]);
+    });
+
+    it('breaks ties in descending order', function () {
+      runSortTest('desc', [
+        [ 'nginx',   301,  'twitter.com'  ],
+        [ 'nginx',   301,  'facebook.com' ],
+        [ 'nginx',   200,  'twitter.com'  ],
+        [ 'nginx',   200,  'facebook.com' ],
+        [ 'apache',  301,  'twitter.com'  ],
+        [ 'apache',  301,  'facebook.com' ],
+        [ 'apache',  200,  'twitter.com'  ],
+        [ 'apache',  200,  'facebook.com' ]
+      ]);
+    });
+  });
+});