Skip to content

Commit

Permalink
Fix group aggregate with NaN values (#328)
Browse files Browse the repository at this point in the history
The group aggregate should produce an output value even when inputs are NaN.

Fixes #326.

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>
  • Loading branch information
fpetkovski committed Nov 20, 2023
1 parent 998354b commit da65a9d
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 4 deletions.
9 changes: 9 additions & 0 deletions engine/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1114,6 +1114,15 @@ func TestQueriesAgainstOldEngine(t *testing.T) {
http_requests_total{pod="nginx-2"} 2+2x18`,
query: `group by (pod) (http_requests_total)`,
},
{
// Issue https://github.com/thanos-io/promql-engine/issues/326.
name: "group by with NaN values",
load: `
load 30s
http_requests_total{pod="nginx-1", route="/"} 1.00+1.00x4
http_requests_total{pod="nginx-2", route="/"} 1+2.00x4`,
query: `group by (pod, route) (atanh(-{__name__="http_requests_total"} offset -3m4s))`,
},
{
name: "resets",
load: `load 30s
Expand Down
12 changes: 8 additions & 4 deletions execution/aggregate/accumulator.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,11 @@ func (s *sumAcc) Reset(_ float64) {
}

type genericAcc struct {
zeroVal float64
value float64
hasValue bool
zeroVal float64
value float64
hasValue bool
skipNaNs bool

aggregate func(float64, float64) float64
vectorAggregate func([]float64, []*histogram.FloatHistogram) float64
}
Expand Down Expand Up @@ -113,6 +115,7 @@ func groupVecAggregate(_ []float64, _ []*histogram.FloatHistogram) float64 {

func newMaxAcc() *genericAcc {
return &genericAcc{
skipNaNs: true,
zeroVal: math.MinInt64,
aggregate: maxAggregate,
vectorAggregate: maxVecAggregate,
Expand All @@ -121,6 +124,7 @@ func newMaxAcc() *genericAcc {

func newMinAcc() *genericAcc {
return &genericAcc{
skipNaNs: true,
zeroVal: math.MaxInt64,
aggregate: minAggregate,
vectorAggregate: minVecAggregate,
Expand All @@ -136,7 +140,7 @@ func newGroupAcc() *genericAcc {
}

func (g *genericAcc) Add(v float64, _ *histogram.FloatHistogram) {
if math.IsNaN(v) {
if g.skipNaNs && math.IsNaN(v) {
return
}
if !g.hasValue {
Expand Down

0 comments on commit da65a9d

Please sign in to comment.