From d8af290947cd488f99f74737b70929b2a1a20c51 Mon Sep 17 00:00:00 2001
From: Aliaksandr Valialkin <valyala@gmail.com>
Date: Tue, 13 Oct 2020 11:56:53 +0300
Subject: [PATCH] app/vmselect/promql: fix `mode_over_time` calculations

Previously `mode_over_time` could return garbage due to improper shuffling of input data points.
---
 CHANGELOG.md                       |  1 +
 app/vmselect/promql/aggr.go        |  2 ++
 app/vmselect/promql/rollup.go      | 18 +++++++++++++++++-
 app/vmselect/promql/rollup_test.go |  6 +++---
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index cf37cfcad..d2f92b380 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -52,6 +52,7 @@
   See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/674
 
 * BUGFIX: vmalert: accept days, weeks and years in `for: ` part of config like Prometheus does. See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/817
+* BUGFIX: fix `mode_over_time(m[d])` calculations. Previously the function could return incorrect results.
 
 
 # [v1.43.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.43.0)
diff --git a/app/vmselect/promql/aggr.go b/app/vmselect/promql/aggr.go
index e38b8dd77..b87b34a64 100644
--- a/app/vmselect/promql/aggr.go
+++ b/app/vmselect/promql/aggr.go
@@ -494,6 +494,8 @@ func aggrFuncZScore(afa *aggrFuncArg) ([]*timeseries, error) {
 //
 // It is expected that a doesn't contain NaNs.
 //
+// The function modifies contents for a, so the caller must prepare it accordingly.
+//
 // See https://en.wikipedia.org/wiki/Mode_(statistics)
 func modeNoNaNs(prevValue float64, a []float64) float64 {
 	if len(a) == 0 {
diff --git a/app/vmselect/promql/rollup.go b/app/vmselect/promql/rollup.go
index 3c34d4301..56d26e2c5 100644
--- a/app/vmselect/promql/rollup.go
+++ b/app/vmselect/promql/rollup.go
@@ -1587,7 +1587,23 @@ func rollupTimestamp(rfa *rollupFuncArg) float64 {
 func rollupModeOverTime(rfa *rollupFuncArg) float64 {
 	// There is no need in handling NaNs here, since they must be cleaned up
 	// before calling rollup funcs.
-	return modeNoNaNs(rfa.prevValue, rfa.values)
+
+	// Copy rfa.values to a.A, since modeNoNaNs modifies a.A contents.
+	a := float64sPool.Get().(*float64s)
+	a.A = append(a.A[:0], rfa.values...)
+	result := modeNoNaNs(rfa.prevValue, a.A)
+	float64sPool.Put(a)
+	return result
+}
+
+var float64sPool = &sync.Pool{
+	New: func() interface{} {
+		return &float64s{}
+	},
+}
+
+type float64s struct {
+	A []float64
 }
 
 func rollupAscentOverTime(rfa *rollupFuncArg) float64 {
diff --git a/app/vmselect/promql/rollup_test.go b/app/vmselect/promql/rollup_test.go
index d698d8c8e..b66136317 100644
--- a/app/vmselect/promql/rollup_test.go
+++ b/app/vmselect/promql/rollup_test.go
@@ -1012,7 +1012,7 @@ func TestRollupFuncsNoWindow(t *testing.T) {
 		}
 		rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step)
 		values := rc.Do(nil, testValues, testTimestamps)
-		valuesExpected := []float64{nan, nan, 34, 44, 44}
+		valuesExpected := []float64{nan, 21, 34, 34, 34}
 		timestampsExpected := []int64{0, 40, 80, 120, 160}
 		testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected)
 	})
@@ -1026,7 +1026,7 @@ func TestRollupFuncsNoWindow(t *testing.T) {
 		}
 		rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step)
 		values := rc.Do(nil, testValues, testTimestamps)
-		valuesExpected := []float64{nan, 1262.5, 3187.5, 4059.523809523809, 6200}
+		valuesExpected := []float64{nan, 2775, 5262.5, 3678.5714285714284, 2880}
 		timestampsExpected := []int64{0, 40, 80, 120, 160}
 		testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected)
 	})
@@ -1040,7 +1040,7 @@ func TestRollupFuncsNoWindow(t *testing.T) {
 		}
 		rc.Timestamps = getTimestamps(rc.Start, rc.End, rc.Step)
 		values := rc.Do(nil, testValues, testTimestamps)
-		valuesExpected := []float64{nan, 0.9397878236968458, 1.1969836716333457, 2.3112921116373175, nan}
+		valuesExpected := []float64{nan, -0.86650328627136, -1.1200838283548589, -0.40035755084856683, nan}
 		timestampsExpected := []int64{0, 40, 80, 120, 160}
 		testRowsEqual(t, values, rc.Timestamps, valuesExpected, timestampsExpected)
 	})