From fd6bdf52307752fb0c70e4c258f66c67e953b091 Mon Sep 17 00:00:00 2001 From: Charles Korn Date: Fri, 28 Jun 2024 11:19:27 +1000 Subject: [PATCH] Fix issue where summation of +/- infinity returns NaN instead of infinity Signed-off-by: Charles Korn --- promql/functions.go | 8 +- promql/functions_internal_test.go | 81 +++++++++++++++++++ promql/promqltest/testdata/aggregators.test | 29 +++++++ .../testdata/native_histograms.test | 4 +- 4 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 promql/functions_internal_test.go diff --git a/promql/functions.go b/promql/functions.go index 9b3be2287..dec3abc25 100644 --- a/promql/functions.go +++ b/promql/functions.go @@ -950,10 +950,14 @@ func funcTimestamp(vals []parser.Value, args parser.Expressions, enh *EvalNodeHe func kahanSumInc(inc, sum, c float64) (newSum, newC float64) { t := sum + inc + switch { + case math.IsInf(t, 0): + c = 0 + // Using Neumaier improvement, swap if next term larger than sum. - if math.Abs(sum) >= math.Abs(inc) { + case math.Abs(sum) >= math.Abs(inc): c += (sum - t) + inc - } else { + default: c += (inc - t) + sum } return t, c diff --git a/promql/functions_internal_test.go b/promql/functions_internal_test.go new file mode 100644 index 000000000..85c6cd797 --- /dev/null +++ b/promql/functions_internal_test.go @@ -0,0 +1,81 @@ +// Copyright 2015 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package promql + +import ( + "fmt" + "math" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestKahanSumInc(t *testing.T) { + testCases := map[string]struct { + first float64 + second float64 + expected float64 + }{ + "+Inf + anything = +Inf": { + first: math.Inf(1), + second: 2.0, + expected: math.Inf(1), + }, + "-Inf + anything = -Inf": { + first: math.Inf(-1), + second: 2.0, + expected: math.Inf(-1), + }, + "+Inf + -Inf = NaN": { + first: math.Inf(1), + second: math.Inf(-1), + expected: math.NaN(), + }, + "NaN + anything = NaN": { + first: math.NaN(), + second: 2, + expected: math.NaN(), + }, + "NaN + Inf = NaN": { + first: math.NaN(), + second: math.Inf(1), + expected: math.NaN(), + }, + "NaN + -Inf = NaN": { + first: math.NaN(), + second: math.Inf(-1), + expected: math.NaN(), + }, + } + + runTest := func(t *testing.T, a, b, expected float64) { + t.Run(fmt.Sprintf("%v + %v = %v", a, b, expected), func(t *testing.T) { + sum, c := kahanSumInc(b, a, 0) + result := sum + c + + if math.IsNaN(expected) { + require.Truef(t, math.IsNaN(result), "expected result to be NaN, but got %v (from %v + %v)", result, sum, c) + } else { + require.Equalf(t, expected, result, "expected result to be %v, but got %v (from %v + %v)", expected, result, sum, c) + } + }) + } + + for name, testCase := range testCases { + t.Run(name, func(t *testing.T) { + runTest(t, testCase.first, testCase.second, testCase.expected) + runTest(t, testCase.second, testCase.first, testCase.expected) + }) + } +} diff --git a/promql/promqltest/testdata/aggregators.test b/promql/promqltest/testdata/aggregators.test index be689c65f..2a0ce01ee 100644 --- a/promql/promqltest/testdata/aggregators.test +++ b/promql/promqltest/testdata/aggregators.test @@ -511,10 +511,39 @@ load 10s data{test="ten",point="b"} 8 data{test="ten",point="c"} 1e+100 data{test="ten",point="d"} -1e100 + data{test="pos_inf",group="1",point="a"} Inf + data{test="pos_inf",group="1",point="b"} 2 + data{test="pos_inf",group="2",point="a"} 2 + data{test="pos_inf",group="2",point="b"} Inf + data{test="neg_inf",group="1",point="a"} -Inf + data{test="neg_inf",group="1",point="b"} 2 + data{test="neg_inf",group="2",point="a"} 2 + data{test="neg_inf",group="2",point="b"} -Inf + data{test="inf_inf",point="a"} Inf + data{test="inf_inf",point="b"} -Inf + data{test="nan",group="1",point="a"} NaN + data{test="nan",group="1",point="b"} 2 + data{test="nan",group="2",point="a"} 2 + data{test="nan",group="2",point="b"} NaN eval instant at 1m sum(data{test="ten"}) {} 10 +eval instant at 1m sum by (group) (data{test="pos_inf"}) + {group="1"} Inf + {group="2"} Inf + +eval instant at 1m sum by (group) (data{test="neg_inf"}) + {group="1"} -Inf + {group="2"} -Inf + +eval instant at 1m sum(data{test="inf_inf"}) + {} NaN + +eval instant at 1m sum by (group) (data{test="nan"}) + {group="1"} NaN + {group="2"} NaN + clear # Test that aggregations are deterministic. diff --git a/promql/promqltest/testdata/native_histograms.test b/promql/promqltest/testdata/native_histograms.test index f79517023..01c2a2157 100644 --- a/promql/promqltest/testdata/native_histograms.test +++ b/promql/promqltest/testdata/native_histograms.test @@ -355,10 +355,10 @@ load 10m histogram_stddev_stdvar_7 {{schema:3 count:7 sum:Inf z_bucket:1 buckets:[0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 1 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 ] n_buckets:[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1 ]}}x1 eval instant at 10m histogram_stddev(histogram_stddev_stdvar_7) - {} NaN + {} Inf eval instant at 10m histogram_stdvar(histogram_stddev_stdvar_7) - {} NaN + {} Inf # Apply quantile function to histogram with all positive buckets with zero bucket. load 10m