From d838a753b8d6a761e85a8297b80f540997074547 Mon Sep 17 00:00:00 2001 From: Rob Skillington Date: Fri, 7 May 2021 04:59:08 -0400 Subject: [PATCH] [query] Fix Graphite nil arg not interpreted as explicit nil (#3481) --- .../graphite/native/builtin_functions.go | 29 ++++------- .../graphite/native/builtin_functions_test.go | 50 ++++++++++++++----- src/query/graphite/native/compiler_test.go | 7 ++- src/query/graphite/native/functions.go | 9 +++- 4 files changed, 60 insertions(+), 35 deletions(-) diff --git a/src/query/graphite/native/builtin_functions.go b/src/query/graphite/native/builtin_functions.go index c4b928c2e9..b3af2e2e48 100644 --- a/src/query/graphite/native/builtin_functions.go +++ b/src/query/graphite/native/builtin_functions.go @@ -1112,24 +1112,15 @@ func asPercent(ctx *common.Context, input singlePathSpec, total genericInterface "require total to be missing, float, single series or same number of series: series=%d, total=%d", len(input.Values), total.Len())) } - if total.Len() == 0 { - // Same as nil take as sum of input. - sum, err := sumSeries(ctx, multiplePathSpecs(input)) - if err != nil { - return ts.NewSeriesList(), err - } - totalSeriesList = sum - } else { - // Sort both by name so they get divided by same name if same length - // or closest match. - sort.Slice(input.Values, func(i, j int) bool { - return strings.Compare(input.Values[i].Name(), input.Values[j].Name()) < 0 - }) - sort.Slice(total.Values, func(i, j int) bool { - return strings.Compare(total.Values[i].Name(), total.Values[j].Name()) < 0 - }) - totalSeriesList = total - } + // Sort both by name so they get divided by same name if same length + // or closest match. + sort.Slice(input.Values, func(i, j int) bool { + return strings.Compare(input.Values[i].Name(), input.Values[j].Name()) < 0 + }) + sort.Slice(total.Values, func(i, j int) bool { + return strings.Compare(total.Values[i].Name(), total.Values[j].Name()) < 0 + }) + totalSeriesList = total default: err := xerrors.NewInvalidParamsError(errors.New( "total must be either an nil, float, a series or same number of series")) @@ -2568,7 +2559,7 @@ func init() { 4: "", // newName }) MustRegisterFunction(asPercent).WithDefaultParams(map[uint8]interface{}{ - 2: []*ts.Series(nil), // total + 2: nil, // total }) MustRegisterFunction(averageAbove) MustRegisterFunction(averageSeries) diff --git a/src/query/graphite/native/builtin_functions_test.go b/src/query/graphite/native/builtin_functions_test.go index 2332c05a2f..c56495fa93 100644 --- a/src/query/graphite/native/builtin_functions_test.go +++ b/src/query/graphite/native/builtin_functions_test.go @@ -36,7 +36,6 @@ import ( "github.com/m3db/m3/src/query/graphite/ts" querystorage "github.com/m3db/m3/src/query/storage" "github.com/m3db/m3/src/query/storage/m3/consolidators" - xerrors "github.com/m3db/m3/src/x/errors" xgomock "github.com/m3db/m3/src/x/test" "github.com/golang/mock/gomock" @@ -2460,21 +2459,36 @@ func TestAsPercentWithSeriesListAndEmptyTotalSeriesList(t *testing.T) { ctx := common.NewTestContext() defer func() { _ = ctx.Close() }() - nan := math.NaN() inputs := []struct { name string step int values []float64 }{ { - "foo.value", + "foo.bar", 100, - []float64{12.0, 14.0, 16.0, nan, 20.0, 30.0}, + []float64{2.5, 5, 7.5, 10}, }, { - "bar.value", - 200, - []float64{7.0, nan, 25.0}, + "foo.baz", + 100, + []float64{10, 20, 30, 40}, + }, + } + outputs := []struct { + name string + step int + values []float64 + }{ + { + "asPercent(foo.bar,sumSeries(foo.*))", + 100, + []float64{20, 20, 20, 20}, + }, + { + "asPercent(foo.baz,sumSeries(foo.*))", + 100, + []float64{80, 80, 80, 80}, }, } @@ -2486,16 +2500,26 @@ func TestAsPercentWithSeriesListAndEmptyTotalSeriesList(t *testing.T) { ctx.StartTime, common.NewTestSeriesValues(ctx, input.step, input.values), ) + timeSeries.Specification = "foo.*" inputSeries = append(inputSeries, timeSeries) } - _, err := asPercent(ctx, singlePathSpec{ + var expected []*ts.Series // nolint: prealloc + for _, output := range outputs { + timeSeries := ts.NewSeries( + ctx, + output.name, + ctx.StartTime, + common.NewTestSeriesValues(ctx, output.step, output.values), + ) + expected = append(expected, timeSeries) + } + + r, err := asPercent(ctx, singlePathSpec{ Values: inputSeries, - }, singlePathSpec{ - Values: []*ts.Series{}, - }) - require.Error(t, err) - require.True(t, xerrors.IsInvalidParams(err)) + }, nil) + require.NoError(t, err) + requireEqual(t, expected, r.Values) } func TestAsPercentWithNodesAndTotalNil(t *testing.T) { diff --git a/src/query/graphite/native/compiler_test.go b/src/query/graphite/native/compiler_test.go index 16edd9943a..006edb81e6 100644 --- a/src/query/graphite/native/compiler_test.go +++ b/src/query/graphite/native/compiler_test.go @@ -406,7 +406,12 @@ func assertExprTree(t *testing.T, expected interface{}, actual interface{}, msg case constFuncArg: a, ok := actual.(constFuncArg) require.True(t, ok, msg) - xtest.Equalish(t, e.value.Interface(), a.value.Interface(), msg) + if !a.value.IsValid() { + // Explicit nil. + require.True(t, e.value.IsZero()) + } else { + xtest.Equalish(t, e.value.Interface(), a.value.Interface(), msg) + } default: assert.Equal(t, expected, actual, msg) } diff --git a/src/query/graphite/native/functions.go b/src/query/graphite/native/functions.go index a5bb9b618b..8dd5a8a935 100644 --- a/src/query/graphite/native/functions.go +++ b/src/query/graphite/native/functions.go @@ -444,6 +444,11 @@ func (f *Function) reflectCall(ctx *common.Context, args []reflect.Value) (refle // Cast to the expected typealias type of ts.SeriesList before calling for i, arg := range in { + if !arg.IsValid() { + // Zero value arg is a nil. + in[i] = reflect.New(genericInterfaceType).Elem() + continue + } typeArg := arg.Type() if typeArg != seriesListType { continue @@ -625,12 +630,12 @@ func (call *functionCall) String() string { // isTimeSeries checks whether the given value contains a timeseries or // timeseries list func isTimeSeries(v reflect.Value) bool { - return v.Type() == seriesListType + return v.IsValid() && v.Type() == seriesListType } // getStats gets trace stats for the given timeseries argument func getStats(v reflect.Value) common.TraceStats { - if v.Type() == timeSeriesType { + if v.IsValid() && v.Type() == timeSeriesType { return common.TraceStats{NumSeries: 1} }