From c0913ce7182194553d83b6ffa08a19e9b07a5609 Mon Sep 17 00:00:00 2001 From: Yuri Tseretyan Date: Fri, 20 Jan 2023 09:53:03 -0500 Subject: [PATCH] SSE: Fix math expression to support NoData results (#61721) * update perFloat to support NoData * update union to correctly handle no-data --- pkg/expr/mathexp/exp.go | 14 ++++ pkg/expr/mathexp/exp_nan_null_val_test.go | 88 +++++++++++++++++++++++ pkg/expr/mathexp/funcs.go | 4 ++ pkg/expr/mathexp/types.go | 4 ++ pkg/expr/mathexp/union_test.go | 32 ++++++++- 5 files changed, 141 insertions(+), 1 deletion(-) diff --git a/pkg/expr/mathexp/exp.go b/pkg/expr/mathexp/exp.go index b772969d371..45e2113d077 100644 --- a/pkg/expr/mathexp/exp.go +++ b/pkg/expr/mathexp/exp.go @@ -7,6 +7,7 @@ import ( "runtime" "github.com/grafana/grafana-plugin-sdk-go/data" + "github.com/grafana/grafana/pkg/expr/mathexp/parse" ) @@ -201,6 +202,11 @@ func union(aResults, bResults Results) []*Union { } if aValueLen == 1 || bValueLen == 1 { if aResults.Values[0].Type() == parse.TypeNoData || bResults.Values[0].Type() == parse.TypeNoData { + unions = append(unions, &Union{ + Labels: nil, + A: aResults.Values[0], + B: bResults.Values[0], + }) return unions } } @@ -285,6 +291,8 @@ func (e *State) walkBinary(node *parse.BinaryNode) (Results, error) { // Scalar op Series case Series: value, err = e.biSeriesNumber(uni.Labels, node.OpStr, bt, aFloat, false) + case NoData: + value = uni.B default: return res, fmt.Errorf("not implemented: binary %v on %T and %T", node.OpStr, uni.A, uni.B) } @@ -301,6 +309,8 @@ func (e *State) walkBinary(node *parse.BinaryNode) (Results, error) { // case Series op Series case Series: value, err = e.biSeriesSeries(uni.Labels, node.OpStr, at, bt) + case NoData: + value = uni.B default: return res, fmt.Errorf("not implemented: binary %v on %T and %T", node.OpStr, uni.A, uni.B) } @@ -315,9 +325,13 @@ func (e *State) walkBinary(node *parse.BinaryNode) (Results, error) { value, err = e.biScalarNumber(uni.Labels, node.OpStr, at, bFloat, true) case Series: value, err = e.biSeriesNumber(uni.Labels, node.OpStr, bt, aFloat, false) + case NoData: + value = uni.B default: return res, fmt.Errorf("not implemented: binary %v on %T and %T", node.OpStr, uni.A, uni.B) } + case NoData: + value = uni.A default: return res, fmt.Errorf("not implemented: binary %v on %T and %T", node.OpStr, uni.A, uni.B) } diff --git a/pkg/expr/mathexp/exp_nan_null_val_test.go b/pkg/expr/mathexp/exp_nan_null_val_test.go index 7c0e8fbd99f..4d928d405a0 100644 --- a/pkg/expr/mathexp/exp_nan_null_val_test.go +++ b/pkg/expr/mathexp/exp_nan_null_val_test.go @@ -1,6 +1,7 @@ package mathexp import ( + "fmt" "math" "testing" "time" @@ -8,6 +9,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/grafana/grafana-plugin-sdk-go/data" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNaN(t *testing.T) { @@ -422,3 +424,89 @@ func TestNullValues(t *testing.T) { }) } } + +func TestNoData(t *testing.T) { + t.Run("unary operation return NoData if input NoData", func(t *testing.T) { + unaryOps := []string{ + "abs($A)", + "is_inf($A)", + "is_nan($A)", + "is_null($A)", + "is_number($A)", + "log($A)", + "round($A)", + "ceil($A)", + "floor($A)", + "!$A", + "-$A", + } + vars := Vars{"A": Results{[]Value{NewNoData()}}} + for _, expr := range unaryOps { + t.Run(fmt.Sprintf("op: %s", expr), func(t *testing.T) { + e, err := New(expr) + require.NoError(t, err) + if e != nil { + res, err := e.Execute("", vars) + require.NoError(t, err) + require.Len(t, res.Values, 1) + require.Equal(t, NewNoData(), res.Values[0]) + } + }) + } + }) + + makeVars := func(a, b Value) Vars { + return Vars{ + "A": Results{[]Value{a}}, + "B": Results{[]Value{b}}, + } + } + + bin_ops := []string{ + "$A || $B", + "$A && $B", + "$A + $B", + "$A * $B", + "$A - $B", + "$A / $B", + "$A ** $B", + "$A % $B", + "$A == $B", + "$A > $B", + "$A != $B", + "$A < $B", + "$A >= $B", + "$A <= $B", + "$A || $B", + "$A && $B", + } + series := makeSeries("test", nil, tp{time.Unix(5, 0), float64Pointer(2)}) + for _, expr := range bin_ops { + t.Run(fmt.Sprintf("op: %s", expr), func(t *testing.T) { + e, err := New(expr) + require.NoError(t, err) + if e != nil { + t.Run("$A,$B=nodata", func(t *testing.T) { + res, err := e.Execute("", makeVars(NewNoData(), NewNoData())) + require.NoError(t, err) + require.Len(t, res.Values, 1) + require.Equal(t, NewNoData(), res.Values[0]) + }) + + t.Run("$A=nodata, $B=series", func(t *testing.T) { + res, err := e.Execute("", makeVars(NewNoData(), series)) + require.NoError(t, err) + require.Len(t, res.Values, 1) + require.Equal(t, NewNoData(), res.Values[0]) + }) + + t.Run("$A=series, $B=nodata", func(t *testing.T) { + res, err := e.Execute("", makeVars(NewNoData(), series)) + require.NoError(t, err) + require.Len(t, res.Values, 1) + require.Equal(t, NewNoData(), res.Values[0]) + }) + } + }) + } +} diff --git a/pkg/expr/mathexp/funcs.go b/pkg/expr/mathexp/funcs.go index a299e3462e5..103dbd32978 100644 --- a/pkg/expr/mathexp/funcs.go +++ b/pkg/expr/mathexp/funcs.go @@ -229,6 +229,8 @@ func perFloat(e *State, val Value, floatF func(x float64) float64) (Value, error newSeries.SetPoint(i, t, &nF) } newVal = newSeries + case parse.TypeNoData: + newVal = NewNoData() default: // TODO: Should we deal with TypeString, TypeVariantSet? } @@ -258,6 +260,8 @@ func perNullableFloat(e *State, val Value, floatF func(x *float64) *float64) (Va newSeries.SetPoint(i, t, floatF(f)) } newVal = newSeries + case parse.TypeNoData: + newVal = NewNoData() default: // TODO: Should we deal with TypeString, TypeVariantSet? } diff --git a/pkg/expr/mathexp/types.go b/pkg/expr/mathexp/types.go index 3e020d85d5a..4c522a670ed 100644 --- a/pkg/expr/mathexp/types.go +++ b/pkg/expr/mathexp/types.go @@ -212,5 +212,9 @@ func (s NoData) AddNotice(notice data.Notice) { func (s NoData) AsDataFrame() *data.Frame { return s.Frame } func (s NoData) New() NoData { + return NewNoData() +} + +func NewNoData() NoData { return NoData{data.NewFrame("no data")} } diff --git a/pkg/expr/mathexp/union_test.go b/pkg/expr/mathexp/union_test.go index da1c4ff5aa6..9aa0f7f1eed 100644 --- a/pkg/expr/mathexp/union_test.go +++ b/pkg/expr/mathexp/union_test.go @@ -103,7 +103,13 @@ func Test_union(t *testing.T) { }, }, unionsAre: assert.EqualValues, - unions: []*Union{}, + unions: []*Union{ + { + Labels: nil, + A: makeSeries("a", data.Labels{"id": "1"}), + B: NewNoData(), + }, + }, }, { name: "incompatible tags of different length with will result in no unions when len(A) != 1 && len(B) != 1", @@ -260,6 +266,30 @@ func Test_union(t *testing.T) { }, }, }, + { + name: "A is no-data and B is anything makes no-data", + // Is this the behavior we want? A result within the results will no longer + // be uniquely identifiable. + aResults: Results{ + Values: Values{ + NewNoData(), + }, + }, + bResults: Results{ + Values: Values{ + makeSeries("a", data.Labels{"id": "1"}), + makeSeries("aa", data.Labels{"id": "1", "fish": "herring"}), + }, + }, + unionsAre: assert.EqualValues, + unions: []*Union{ + { + Labels: nil, + A: NewNoData(), + B: makeSeries("a", data.Labels{"id": "1"}), + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {