Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[query] Allow Graphite variadic functions to omit variadic args #2882

Merged
merged 5 commits into from
Nov 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions site/.htmltest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,7 @@ IgnoreURLs:
- "youtu.be"
- "youtube.com"
- "cassandra.apache.org"
- "slideshare.net"
- "meetup.com"
- "github.com/m3db/m3/tree/docs-test/site/content/docs"
- "github.com/m3db/m3/tree/master/site/content/docs"
5 changes: 1 addition & 4 deletions src/query/graphite/native/builtin_functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -2375,7 +2375,6 @@ func init() {
})
MustRegisterFunction(asPercent).WithDefaultParams(map[uint8]interface{}{
2: []*ts.Series(nil), // total
3: nil, // nodes
})
MustRegisterFunction(averageAbove)
MustRegisterFunction(averageSeries)
Expand Down Expand Up @@ -2406,9 +2405,7 @@ func init() {
MustRegisterFunction(groupByNode).WithDefaultParams(map[uint8]interface{}{
3: "average", // fname
})
MustRegisterFunction(groupByNodes).WithDefaultParams(map[uint8]interface{}{
3: nil, // nodes
})
MustRegisterFunction(groupByNodes)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both asPercent and groupByNodes functions will need to be updated to take in a multiplePathSpecs instead of a singlePathSpec so they'd get properly cast as variadic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, they are already variadic due to nodes ...int third arg.

I added a test which was returning an error before and now passes in engine_test.go 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nice, may have to remove aliasByNode(foo.*.zed) case from TestCompileErrors then 👍

MustRegisterFunction(highest).WithDefaultParams(map[uint8]interface{}{
2: 1, // n,
3: "average", // f
Expand Down
7 changes: 6 additions & 1 deletion src/query/graphite/native/compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ func (c *compiler) compileFunctionCall(fname string, nextToken *lexer.Token) (*f
}

argTypes := fn.in
argTypesRequired := len(fn.in)
if fn.variadic {
// Variadic can avoid specifying the last arg.
argTypesRequired--
}
var args []funcArg

// build up arguments for function call
Expand Down Expand Up @@ -206,7 +211,7 @@ func (c *compiler) compileFunctionCall(fname string, nextToken *lexer.Token) (*f
}

// all required argument types should be filled with values now
if len(args) < len(argTypes) {
if len(args) < argTypesRequired {
variadicComment := ""
if fn.variadic {
variadicComment = "at least "
Expand Down
6 changes: 0 additions & 6 deletions src/query/graphite/native/compiler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,6 @@ func TestCompileErrors(t *testing.T) {
{"aliasByNode()",
"invalid expression 'aliasByNode()': invalid number of arguments for aliasByNode;" +
" expected at least 2, received 0"},
{"aliasByNode(foo.*.zed)", // check that at least 1 param is provided for variadic args
"invalid expression 'aliasByNode(foo.*.zed)': invalid number of arguments for " +
"aliasByNode; expected at least 2, received 1"},
{"aliasByNode(foo.*.zed, 2, false)",
"invalid expression 'aliasByNode(foo.*.zed, 2, false)': invalid function call " +
"aliasByNode, arg 2: expected a int, received 'false'"},
Expand All @@ -327,9 +324,6 @@ func TestCompileErrors(t *testing.T) {
{"summarize(foo.bar.baz.quux)",
"invalid expression 'summarize(foo.bar.baz.quux)':" +
" invalid number of arguments for summarize; expected 4, received 1"},
{"sumSeries()", // check that at least 1 series is provided for variadic timeSeriesList
"invalid expression 'sumSeries()': invalid number of arguments for sumSeries;" +
" expected at least 1, received 0"},
{"sumSeries(foo.bar.baz.quux,)",
"invalid expression 'sumSeries(foo.bar.baz.quux,)': invalid function call sumSeries, " +
"arg 1: invalid expression 'sumSeries(foo.bar.baz.quux,)': ) not valid"},
Expand Down
10 changes: 9 additions & 1 deletion src/query/graphite/native/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ func TestExecute(t *testing.T) {
{"foo.bar.q.zed", "foo.q", 0},
{"foo.bar.x.zed", "foo.x", 2},
}},
{"groupByNodes(foo.bar.*.zed, \"sum\")", false, []queryTestResult{
{"foo.bar.*.zed", "foo.bar.*.zed", 3},
}},
{"groupByNodes(foo.bar.*.zed, \"sum\", 2)", false, []queryTestResult{
{"foo.bar.q.zed", "foo.bar.q.zed", 0},
{"foo.bar.g.zed", "foo.bar.g.zed", 1},
{"foo.bar.x.zed", "foo.bar.x.zed", 2},
}},
}

ctx := common.NewContext(common.ContextOptions{Start: time.Now().Add(-1 * time.Hour), End: time.Now(), Engine: engine})
Expand All @@ -140,7 +148,7 @@ func TestExecute(t *testing.T) {
buildTestSeriesFn(stepSize, queries...))

expr, err := engine.Compile(test.query)
require.Nil(t, err)
require.NoError(t, err)

results, err := expr.Execute(ctx)
require.Nil(t, err, "failed to execute %s", test.query)
Expand Down
7 changes: 6 additions & 1 deletion src/query/graphite/native/functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,12 @@ func (f *Function) reflectCall(ctx *common.Context, args []reflect.Value) (refle
}

numTypes := len(f.in)
if len(in) < numTypes {
numRequiredTypes := numTypes
if f.variadic {
// Variadic can avoid specifying the last arg.
numRequiredTypes--
}
if len(in) < numRequiredTypes {
err := fmt.Errorf("call args mismatch: expected at least %d, actual %d",
len(f.in), len(in))
return reflect.Value{}, err
Expand Down