[query] Fix variadic functions #1846

arnikola · 2019-07-31T23:12:16Z

Special notes for your reviewer: bit of scope creep; this PR also fixes a couple of incorrect functions, and lazifies some others.

robskillington · 2019-08-01T02:21:25Z

src/query/functions/linear/absent.go

@@ -26,6 +26,8 @@ import (
 	"github.com/m3db/m3/src/query/executor/transform"
 )

+// FIXME: This is incorrect functionality.
+//' This should be an aggregation function that works in a step-wise fashion.


Can we create an issue for this?

Sure; actually did the impl for this during the PR but didn't want to pollute this with unrelated stuff even further haha; maybe I'll just jam it in here too, it's actually tiny

Ah damn actually a bit trickier than what I had since it's not quite a "standard" aggregation (i.e. it creates its own series if one hasn't been passed in); will add an issue for now

robskillington · 2019-08-01T02:22:55Z

src/query/functions/linear/datetime.go

-	"github.com/m3db/m3/src/query/executor/transform"
+	"github.com/m3db/m3/src/query/block"
+	"github.com/m3db/m3/src/query/functions/lazy"
+


nit: Join the imports?

Good call; weird that the linter didn't break the build on this though

robskillington · 2019-08-01T02:24:45Z

src/query/functions/linear/datetime.go

-func NewDateOp(optype string) (BaseOp, error) {
-	if _, ok := datetimeFuncs[optype]; !ok {
-		return emptyOp, fmt.Errorf("unknown date type: %s", optype)
+func buildTransform(args []interface{}, fn timeFn) block.ValueTransform {


Hm is args used in this method at all? Seems like only checked if length == 0?

Yeah that's pretty much it; the way this family of functions work is that you can call them with 0 or 1 args, but either way it doesn't actually care about the value of the arg, just that it's there. I'll refactor this a bit to make it a bit nicer and clearer though (this will also address your comment re: n.Func.Name != linear.RoundType)

robskillington · 2019-08-01T02:27:14Z

src/query/functions/linear/round.go

-		processorFn:  makeRoundProcessor(spec),
-	}, nil
-
+	return 1, nil


Interesting, why does args > 1 always return a 1 value? Should this be error instead or no?

So if there's more than 1 arg it will error out, 1 arg will attempt to parse the float, and this path is for 0 args which just returns the default value; will add a comment explaining this 👍

robskillington · 2019-08-01T02:33:06Z

src/query/parser/promql/parse.go

+			}
+
+			if argCount != exprCount {
+				numVals--


Maybe we should call this numExpVals or something to reflect it's what we're expecting?

robskillington · 2019-08-01T02:34:03Z

src/query/parser/promql/parse.go

+					"function %q, received %d", exprCount, n.Func.Name, argCount)
+			}
+
+			if argCount != exprCount {


For readability, does it make sense to do something like if argCount-1 == exprCount { ... } do this logic?

Sorry, don't quite get what you mean here

robskillington · 2019-08-01T02:35:18Z

src/query/parser/promql/parse.go

@@ -246,6 +273,10 @@ func (p *parseState) walk(node pql.Node) error {
 			} else {
 				if e, ok := expr.(*pql.MatrixSelector); ok {
 					argValues = append(argValues, e.Range)
+				} else if _, ok := expr.(*pql.VectorSelector); ok {
+					if variadic != 0 && n.Func.Name != linear.RoundType {


Hm weird, what's special about not being round type?

It's actually more like we should only add it for date functions (which can have an arg that we don't actually care about), and it's using knowledge of prom internals (the functions with Variadic == 1 are the date ones and the round type) which is probably a little naughty haha

Will refactor slightly to make this clear and less janky

codecov · 2019-08-01T14:09:43Z

Codecov Report

Merging #1846 into master will increase coverage by <.1%.
The diff coverage is 74.4%.

@@           Coverage Diff            @@
##           master   #1846     +/-   ##
========================================
+ Coverage    72.2%   72.3%   +<.1%     
========================================
  Files         987     986      -1     
  Lines       83822   83545    -277     
========================================
- Hits        60556   60411    -145     
+ Misses      19237   19127    -110     
+ Partials     4029    4007     -22

Flag	Coverage Δ
#aggregator	`82.4% <ø> (ø)`	⬆️
#cluster	`85.5% <ø> (ø)`	⬆️
#collector	`63.7% <ø> (ø)`	⬆️
#dbnode	`79.8% <ø> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.2% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.5% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (+0.1%)`	⬆️
#query	`68.1% <74.4%> (+0.3%)`	⬆️
#x	`85.4% <ø> (-0.1%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ffd32d...6b2d411. Read the comment docs.

robskillington · 2019-08-02T17:14:03Z

src/query/functions/linear/round.go

-	if len(args) > 1 {
-		return emptyOp, fmt.Errorf("invalid number of args for round: %d", len(args))
+func parseArgs(args []interface{}) (float64, error) {
+	// NB: if no args passed; this should use default value of `1`.


Nice, this definitely reads more clearer, good stuff.

robskillington · 2019-08-02T17:16:16Z

src/query/parser/promql/parse_test.go

+	q := `label_join(up, "foo", ",")`
+	p, err := Parse(q, models.NewTagOptions())
+	require.NoError(t, err)
+	_, _, _ = p.DAG()


nit: You could do assert.NotPanics(t, func() { _, _, _ = p.DAG() }) to be explicit about intent of calling this method (slighty nicer output if it does actually panic too, keeps running rest of the tests in the module rather than stopping whole test run).

Oh that's pretty cool, makes sense it's an inbuilt

robskillington · 2019-08-02T17:16:49Z

src/query/parser/promql/types.go

 // NewFunctionExpr creates a new function expr based on the type.
 func NewFunctionExpr(
 	name string,
 	argValues []interface{},
 	stringValues []string,
+	hasArgValue bool,
 	tagOptions models.TagOptions,
 ) (parser.Params, bool, error) {
 	var p parser.Params
 	var err error


nit: Combine this into single var block?

var ( p parser.Params err error )

robskillington

LGTM other than nits

Got updated in #1745 but reverted in #1846.

arnikola added 2 commits July 31, 2019 19:03

[query] Fix variadic functions

bed90dd

Fix for rounding function

392d3ca

robskillington reviewed Aug 1, 2019

View reviewed changes

PR response

6b2d411

robskillington reviewed Aug 2, 2019

View reviewed changes

robskillington approved these changes Aug 2, 2019

View reviewed changes

arnikola added 2 commits August 2, 2019 14:06

Merge branch 'master' into arnikola/variadic-functions

d517036

pr response

b306887

arnikola merged commit ba60c45 into master Aug 2, 2019

arnikola deleted the arnikola/variadic-functions branch August 2, 2019 18:46

schallert added a commit that referenced this pull request Aug 2, 2019

[ci] fix ci-scripts submodule

78df412

Got updated in #1745 but reverted in #1846.

schallert mentioned this pull request Aug 2, 2019

[ci] fix ci-scripts submodule #1851

Merged

schallert added a commit that referenced this pull request Aug 2, 2019

[ci] fix ci-scripts submodule (#1851)

0b5c7dd

Got updated in #1745 but reverted in #1846.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Fix variadic functions #1846

[query] Fix variadic functions #1846

arnikola commented Jul 31, 2019

robskillington Aug 1, 2019

arnikola Aug 1, 2019

arnikola Aug 1, 2019

robskillington Aug 1, 2019

arnikola Aug 1, 2019

robskillington Aug 1, 2019

arnikola Aug 1, 2019

robskillington Aug 1, 2019

arnikola Aug 1, 2019

robskillington Aug 1, 2019

robskillington Aug 1, 2019

arnikola Aug 1, 2019

robskillington Aug 1, 2019

arnikola Aug 1, 2019

codecov bot commented Aug 1, 2019

robskillington Aug 2, 2019

robskillington Aug 2, 2019

arnikola Aug 2, 2019

robskillington Aug 2, 2019

robskillington left a comment

[query] Fix variadic functions #1846

[query] Fix variadic functions #1846

Conversation

arnikola commented Jul 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 1, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment