Enforce predictable order for first() and last()

[otoolep]

When two or more points have the same timestamp during first() and last() aggregations, the tie is broken by taking the one with the larger value.

Solution thanks to @jwilder

[otoolep]

@jwilder @DanielMorsing @dgnorton

[dgnorton]

@otoolep may be a silly question but does the test fail without the fix?

[otoolep]

@dgnorton -- yes it does. Sometimes it passes, sometimes it doesn't. Without the fix:

~/repos/influxdb/src/github.com/influxdb/influxdb/cmd/influxd/run (deterministic_first_last)$ go test -run TestServer_Query_AggregatesIdenticalTime
[wal] 2015/08/24 16:52:28 WAL starting with 30720 ready series size, 0.50 compaction threshold, and 20971520 partition size threshold
[wal] 2015/08/24 16:52:28 WAL writing to /tmp/influxd-028175029/db0/rp0/1
--- FAIL: TestServer_Query_AggregatesIdenticalTime (0.96s)
        server_test.go:2256: last from multiple series with identical timestamp: unexpected results
                query:  SELECT last(value) FROM "series"
                exp:    {"results":[{"series":[{"name":"series","columns":["time","last"],"values":[["1970-01-01T00:00:00Z",1]]}]}]}
                actual: {"results":[{"series":[{"name":"series","columns":["time","last"],"values":[["1970-01-01T00:00:00Z",5]]}]}]}

FAIL
exit status 1
FAIL    github.com/influxdb/influxdb/cmd/influxd/run    0.962s
~/repos/influxdb/src/github.com/influxdb/influxdb/cmd/influxd/run (deterministic_first_last)$ go test -run TestServer_Query_AggregatesIdenticalTime
[wal] 2015/08/24 16:52:35 WAL starting with 30720 ready series size, 0.50 compaction threshold, and 20971520 partition size threshold
[wal] 2015/08/24 16:52:35 WAL writing to /tmp/influxd-205298755/db0/rp0/1
PASS
ok      github.com/influxdb/influxdb/cmd/influxd/run    1.042s
~/repos/influxdb/src/github.com/influxdb/influxdb/cmd/influxd/run (deterministic_first_last)$ 

[otoolep]

Failed first time, passed second time.

[dgnorton]

+1

[otoolep]

@pauldix has agreed that the behaviour should be deterministic. I will profile this and if this change is not a problem I will merge.

[otoolep]

The impact of this change is significant. With our stress tool select count(value) from cpu takes 30% longer to run. There may be another approach.

[otoolep]

ROUTINE ======================== github.com/influxdb/influxdb/tsdb.pointHeap.Less in /home/philip/repos/influxdb/src/github.com/influxdb/influxdb/tsdb/mapper.go
    14.62s     20.87s (flat, cum) 37.00% of Total
         .          .    651:   return &q
         .          .    652:}
         .          .    653:
         .          .    654:func (pq pointHeap) Len() int { return len(pq) }
         .          .    655:
     830ms      830ms    656:func (pq pointHeap) Less(i, j int) bool {
    10.43s     10.43s    657:   if pq[i].timestamp < pq[j].timestamp {
     310ms      310ms    658:           return true
     380ms      380ms    659:   } else if pq[i].timestamp > pq[j].timestamp {
     150ms      150ms    660:           return false
         .          .    661:   }
         .          .    662:   // Break the tie in a deterministic way.
     2.52s      8.77s    663:   return bytes.Compare(pq[i].value, pq[j].value) == -1
         .          .    664:}
         .          .    665:
         .          .    666:func (pq pointHeap) Swap(i, j int) { pq[i], pq[j] = pq[j], pq[i] }
         .          .    667:
         .          .    668:func (pq *pointHeap) Push(x interface{}) {

[otoolep]

Some other ideas:

-- MapLast and MapFirst could return multiple values, as suggested by @dgnorton and @DanielMorsing. @pauldix not thrilled by this idea.
-- The comparison shown in the patch could only kick in if last() or first() is being invoked.

[otoolep]

The general agreement is however that a 20-30% query hit across the board is not acceptable just to deal with this edge-case.

[otoolep]

With this change we now only take the hit for last and first queries, and this is desired functionality.

[dgnorton]

+1