Skip to content

Commit

Permalink
[query] Fix Graphite nil arg not interpreted as explicit nil (#3481)
Browse files Browse the repository at this point in the history
  • Loading branch information
robskillington authored May 7, 2021
1 parent 4df4556 commit d838a75
Show file tree
Hide file tree
Showing 4 changed files with 60 additions and 35 deletions.
29 changes: 10 additions & 19 deletions src/query/graphite/native/builtin_functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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)
Expand Down
50 changes: 37 additions & 13 deletions src/query/graphite/native/builtin_functions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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},
},
}

Expand All @@ -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) {
Expand Down
7 changes: 6 additions & 1 deletion src/query/graphite/native/compiler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
9 changes: 7 additions & 2 deletions src/query/graphite/native/functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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}
}

Expand Down

0 comments on commit d838a75

Please sign in to comment.