diff --git a/CHANGELOG.md b/CHANGELOG.md index cf37cfcad0..d2f92b3804 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 e38b8dd77a..b87b34a64a 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 3c34d43013..56d26e2c53 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 d698d8c8eb..b66136317d 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) })