Merge pull request #1271 from prometheus/topk-nan

Make topk/bottomk prefer returning real numbers over NaN.

promql/functions.go

@@ -194,7 +194,7 @@ func funcTopk(ev *evaluator, args Expressions) model.Value {
 	topk := make(vectorByValueHeap, 0, k)
 
 	for _, el := range vec {
-		if len(topk) < k || topk[0].Value < el.Value {
+		if len(topk) < k || topk[0].Value < el.Value || math.IsNaN(float64(topk[0].Value)) {
 			if len(topk) == k {
 				heap.Pop(&topk)
 			}
@@ -213,18 +213,17 @@ func funcBottomk(ev *evaluator, args Expressions) model.Value {
 	}
 	vec := ev.evalVector(args[1])
 
-	bottomk := make(vectorByValueHeap, 0, k)
-	bkHeap := reverseHeap{Interface: &bottomk}
+	bottomk := make(vectorByReverseValueHeap, 0, k)
 
 	for _, el := range vec {
-		if len(bottomk) < k || bottomk[0].Value > el.Value {
+		if len(bottomk) < k || bottomk[0].Value > el.Value || math.IsNaN(float64(bottomk[0].Value)) {
 			if len(bottomk) == k {
-				heap.Pop(&bkHeap)
+				heap.Pop(&bottomk)
 			}
-			heap.Push(&bkHeap, el)
+			heap.Push(&bottomk, el)
 		}
 	}
-	sort.Sort(bottomk)
+	sort.Sort(sort.Reverse(bottomk))
 	return vector(bottomk)
 }
 
@@ -985,10 +984,31 @@ func (s *vectorByValueHeap) Pop() interface{} {
 	return el
 }
 
-type reverseHeap struct {
-	heap.Interface
+type vectorByReverseValueHeap vector
+
+func (s vectorByReverseValueHeap) Len() int {
+	return len(s)
 }
 
-func (s reverseHeap) Less(i, j int) bool {
-	return s.Interface.Less(j, i)
+func (s vectorByReverseValueHeap) Less(i, j int) bool {
+	if math.IsNaN(float64(s[i].Value)) {
+		return true
+	}
+	return s[i].Value > s[j].Value
+}
+
+func (s vectorByReverseValueHeap) Swap(i, j int) {
+	s[i], s[j] = s[j], s[i]
+}
+
+func (s *vectorByReverseValueHeap) Push(x interface{}) {
+	*s = append(*s, x.(*sample))
+}
+
+func (s *vectorByReverseValueHeap) Pop() interface{} {
+	old := *s
+	n := len(old)
+	el := old[n-1]
+	*s = old[0 : n-1]
+	return el
 }

promql/testdata/functions.test

@@ -193,3 +193,46 @@ eval instant at 0m clamp_max(clamp_min(test_clamp, -20), 70)
 	{src="clamp-a"}	-20
 	{src="clamp-b"}	0
 	{src="clamp-c"}	70
+
+clear
+
+# Tests for topk/bottomk.
+load 5m
+	http_requests{job="api-server", instance="0", group="production"}	0+10x10
+	http_requests{job="api-server", instance="1", group="production"}	0+20x10
+	http_requests{job="api-server", instance="2", group="production"}	NaN NaN NaN NaN NaN NaN NaN NaN NaN NaN
+	http_requests{job="api-server", instance="0", group="canary"}		0+30x10
+	http_requests{job="api-server", instance="1", group="canary"}		0+40x10
+	http_requests{job="app-server", instance="0", group="production"}	0+50x10
+	http_requests{job="app-server", instance="1", group="production"}	0+60x10
+	http_requests{job="app-server", instance="0", group="canary"}		0+70x10
+	http_requests{job="app-server", instance="1", group="canary"}		0+80x10
+
+eval_ordered instant at 50m topk(3, http_requests)
+	http_requests{group="canary", instance="1", job="app-server"} 800
+	http_requests{group="canary", instance="0", job="app-server"} 700
+	http_requests{group="production", instance="1", job="app-server"} 600
+
+eval_ordered instant at 50m topk(5, http_requests{group="canary",job="app-server"})
+	http_requests{group="canary", instance="1", job="app-server"} 800
+	http_requests{group="canary", instance="0", job="app-server"} 700
+
+eval_ordered instant at 50m bottomk(3, http_requests)
+	http_requests{group="production", instance="0", job="api-server"} 100
+	http_requests{group="production", instance="1", job="api-server"} 200
+	http_requests{group="canary", instance="0", job="api-server"} 300
+
+eval_ordered instant at 50m bottomk(5, http_requests{group="canary",job="app-server"})
+	http_requests{group="canary", instance="0", job="app-server"} 700
+	http_requests{group="canary", instance="1", job="app-server"} 800
+
+# Test NaN is sorted away from the top/bottom.
+eval_ordered instant at 50m topk(3, http_requests{job="api-server",group="production"})
+	http_requests{job="api-server", instance="1", group="production"}	200
+	http_requests{job="api-server", instance="0", group="production"}	100
+	http_requests{job="api-server", instance="2", group="production"}	NaN
+
+eval_ordered instant at 50m bottomk(3, http_requests{job="api-server",group="production"})
+	http_requests{job="api-server", instance="0", group="production"}	100
+	http_requests{job="api-server", instance="1", group="production"}	200
+	http_requests{job="api-server", instance="2", group="production"}	NaN

promql/testdata/legacy.test

@@ -164,23 +164,6 @@ eval_ordered instant at 50m sort_desc(http_requests)
 	http_requests{group="production", instance="1", job="api-server"} 200 
 	http_requests{group="production", instance="0", job="api-server"} 100 
 
-eval_ordered instant at 50m topk(3, http_requests)
-	http_requests{group="canary", instance="1", job="app-server"} 800 
-	http_requests{group="canary", instance="0", job="app-server"} 700 
-	http_requests{group="production", instance="1", job="app-server"} 600 
-
-eval_ordered instant at 50m topk(5, http_requests{group="canary",job="app-server"})
-	http_requests{group="canary", instance="1", job="app-server"} 800 
-	http_requests{group="canary", instance="0", job="app-server"} 700 
-
-eval_ordered instant at 50m bottomk(3, http_requests)
-	http_requests{group="production", instance="0", job="api-server"} 100 
-	http_requests{group="production", instance="1", job="api-server"} 200 
-	http_requests{group="canary", instance="0", job="api-server"} 300 
-
-eval_ordered instant at 50m bottomk(5, http_requests{group="canary",job="app-server"})
-	http_requests{group="canary", instance="0", job="app-server"} 700 
-	http_requests{group="canary", instance="1", job="app-server"} 800 
 
 
 # Single-letter label names and values.