execution: fix min/max aggregators (#335)

Signed-off-by: Michael Hoffmann <[email protected]>
thanos-io · Nov 29, 2023 · f3dcefe · f3dcefe
1 parent 6675f2d
commit f3dcefe
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 8 deletions.
diff --git a/engine/engine_test.go b/engine/engine_test.go
@@ -2817,6 +2817,36 @@ func TestInstantQuery(t *testing.T) {
 		queryTime    time.Time
 		sortByLabels bool // if true, the series in the result between the old and new engine should be sorted before compared
 	}{
+		{
+			name: "fuzz - min with NaN",
+			load: `load 30s
+              http_requests_total{pod="nginx-1", route="/"} 124.00+1.00x40
+              http_requests_total{pod="nginx-2", route="/"}  0+0.29x40`,
+			query: "min by (route, pod) (sqrt(-http_requests_total))",
+		},
+		{
+			name: "fuzz - min with Inf",
+			load: `load 30s
+               http_requests_total{pod="nginx-1", route="/"} 483.00+6035.00x40
+            	 http_requests_total{pod="nginx-2", route="/"}  2+47.14x40`,
+			query: `
+min without () (
+              (
+                  {__name__="http_requests_total"} @ start() offset -2m40s
+                ^
+                  {__name__="http_requests_total"} @ start() offset -2m49s
+              )
+            )`,
+		},
+		/*
+		   This is a known issue, we lose the signed 0 in the sum because we add to a
+		   default element. Prometheus assigns the first element to the sum and preserves
+		   the sign.
+		       {
+		         name:  "fuzz - signed zero",
+		         query: `1/sum(-(absent(X)-1))`,
+		       },
+		*/
 		{
 			name: "sum_over_time with subquery",
 			load: `load 10s

diff --git a/execution/aggregate/accumulator.go b/execution/aggregate/accumulator.go
@@ -82,7 +82,6 @@ type genericAcc struct {
 	zeroVal  float64
 	value    float64
 	hasValue bool
-	skipNaNs bool
 
 	aggregate       func(float64, float64) float64
 	vectorAggregate func([]float64, []*histogram.FloatHistogram) float64
@@ -115,17 +114,15 @@ func groupVecAggregate(_ []float64, _ []*histogram.FloatHistogram) float64 {
 
 func newMaxAcc() *genericAcc {
 	return &genericAcc{
-		skipNaNs:        true,
-		zeroVal:         math.MinInt64,
+		zeroVal:         math.Inf(-1),
 		aggregate:       maxAggregate,
 		vectorAggregate: maxVecAggregate,
 	}
 }
 
 func newMinAcc() *genericAcc {
 	return &genericAcc{
-		skipNaNs:        true,
-		zeroVal:         math.MaxInt64,
+		zeroVal:         math.Inf(+1),
 		aggregate:       minAggregate,
 		vectorAggregate: minVecAggregate,
 	}
@@ -140,9 +137,6 @@ func newGroupAcc() *genericAcc {
 }
 
 func (g *genericAcc) Add(v float64, _ *histogram.FloatHistogram) {
-	if g.skipNaNs && math.IsNaN(v) {
-		return
-	}
 	if !g.hasValue {
 		g.value = g.aggregate(g.zeroVal, v)
 		g.hasValue = true