[query] Add support for unary expressions #1647

benraskin92 · 2019-05-20T03:28:22Z

What this PR does / why we need it:

This PR fixes #1619. It introduces support for unary expressions -- specifically + and -. For example, this query is now supported:

-sum(up)

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:

Yes, this PR adds support for Prometheus unary expressions (`+` and `-`).

Does this PR require updating code package or user-facing documentation?:

NONE

codecov · 2019-05-20T03:43:37Z

Codecov Report

Merging #1647 into master will decrease coverage by 0.1%.
The diff coverage is 67.8%.

@@           Coverage Diff            @@
##           master   #1647     +/-   ##
========================================
- Coverage    72.3%   72.1%   -0.2%     
========================================
  Files         962     961      -1     
  Lines       80157   80051    -106     
========================================
- Hits        57967   57783    -184     
- Misses      18398   18497     +99     
+ Partials     3792    3771     -21

Flag	Coverage Δ
#aggregator	`82.3% <ø> (-0.1%)`	⬇️
#cluster	`85.7% <ø> (-0.1%)`	⬇️
#collector	`63.9% <ø> (ø)`	⬆️
#dbnode	`80.2% <ø> (ø)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74.1% <ø> (ø)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.7% <ø> (ø)`	⬆️
#query	`67.3% <67.8%> (-0.7%)`	⬇️
#x	`86.4% <ø> (ø)`	⬆️

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 79ee9f5...7ec1709. Read the comment docs.

codecov · 2019-05-20T03:43:42Z

Codecov Report

Merging #1647 into master will increase coverage by 6.6%.
The diff coverage is 89.1%.

@@           Coverage Diff            @@
##           master   #1647     +/-   ##
========================================
+ Coverage    65.3%   71.9%   +6.6%     
========================================
  Files         958     964      +6     
  Lines       80131   80725    +594     
========================================
+ Hits        52369   58092   +5723     
+ Misses      24080   18827   -5253     
- Partials     3682    3806    +124

Flag	Coverage Δ
#aggregator	`82.4% <ø> (-2.5%)`	⬇️
#cluster	`85.7% <ø> (+15.7%)`	⬆️
#collector	`63.9% <ø> (+8.7%)`	⬆️
#dbnode	`80.1% <ø> (+2.7%)`	⬆️
#m3em	`73.2% <ø> (ø)`	⬆️
#m3ninx	`74% <ø> (+5.2%)`	⬆️
#m3nsch	`51.1% <ø> (ø)`	⬆️
#metrics	`17.6% <ø> (ø)`	⬆️
#msg	`74.9% <ø> (+0.1%)`	⬆️
#query	`66.4% <89.1%> (+19.1%)`	⬆️
#x	`86.3% <ø> (+8.8%)`	⬆️

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 2ca0202...3dac576. Read the comment docs.

prateek · 2019-05-20T20:09:03Z

src/query/block/lazy.go

+		stepVals = c.Values()
+	)
+
+	vals := make([]float64, len(stepVals))


nit: could you use the make([].., 0, ..) formulation and append instead of directly accessing by idx.

Isn't it more efficient to allocate the length up front (instead of capacity) and index in? Or is this a better safe than sorry situation and avoid a potential panic?

can get both by doing the other version.

i.e. when using the version with append (below), it's not expanding the slice unless the initial alloc wasn't large enough. so you still get the benefit of the up front alloc but also the safety of not panic'ing.

slice := make([]type, 0, len) for _, v := range otherThing { slice = append(slice, v) }

Ah okay, yeah that makes sense.

prateek · 2019-05-20T20:09:12Z

src/query/block/lazy.go

+	var (
+		c    = it.it.Current()
+		vt   = it.opts.ValueTransform()
+		vals = make([]float64, len(c.values))


prateek · 2019-05-20T20:09:31Z

src/query/block/lazy.go

+	var (
+		c       = it.it.Current()
+		stepDPs = c.Values()
+		dpList  = make([]ts.Datapoints, len(stepDPs))


here and on line 245 too

arnikola · 2019-05-21T15:01:07Z

src/query/parser/promql/types.go

@@ -293,6 +293,18 @@ func getBinaryOpType(opType promql.ItemType) string {
 	}
 }

+// GetUnaryOpType returns the M3 unary op type based on the Prom op type


nit; full stops at end of comments

arnikola · 2019-05-21T15:02:20Z

src/query/parser/promql/parse.go

+
+		unaryType, ok := GetUnaryOpType(n.Op)
+		if !ok {
+			return fmt.Errorf("only + and - operators allowed for unary expressions, received: %s", n.Op.String())


nit; instead, have GetUnaryOpType return a (unaryType, error) and just return this error there

arnikola · 2019-05-21T15:02:39Z

src/query/parser/promql/types.go

@@ -293,6 +293,18 @@ func getBinaryOpType(opType promql.ItemType) string {
 	}
 }

+// GetUnaryOpType returns the M3 unary op type based on the Prom op type
+func GetUnaryOpType(opType promql.ItemType) (string, bool) {


nit; does this need to be exported?

Nope, good call.

arnikola · 2019-05-21T15:10:39Z

src/query/block/lazy.go

-			val[i].Value = vt(dp.Value)
+	var (
+		c       = it.it.Current()
+		stepDPs = c.Values()


nit; values reads a bit better

arnikola · 2019-05-21T15:11:11Z

src/query/block/lazy.go

-			val[i].Value = vt(dp.Value)
+	var (
+		c         = it.it.Current()
+		seriesDPs = c.datapoints


nit; values reads better

arnikola · 2019-05-21T15:12:07Z

src/query/block/lazy_test.go

@@ -418,3 +419,224 @@ func TestUnconsolidatedSeriesIter(t *testing.T) {

 	assert.Equal(t, expected, actual.Datapoints())
 }
+
+// negative value offset tests
+


nit: newline

arnikola · 2019-05-21T15:15:55Z

src/query/parser/promql/parse.go

+			return fmt.Errorf("only + and - operators allowed for unary expressions, received: %s", n.Op.String())
+		}
+
+		return p.addLazyTransform(0, lazy.UnaryType, unaryType)


This is a bit janky; I think it's better to split the functions up for now (i.e. noops if it's plus, and creates a value transform otherwise)

arnikola · 2019-05-22T19:03:41Z

src/query/parser/promql/types.go

@@ -248,7 +248,7 @@ func NewFunctionExpr(
 		return p, true, err

 	default:
-		// TODO: handle other types
+		// TODO: handle other types.


nit; can probably remove this, think we handle all types

arnikola · 2019-05-22T19:09:51Z

src/query/parser/promql/parse_test.go

@@ -76,6 +77,39 @@ func TestInvalidOffset(t *testing.T) {
 	require.Error(t, err)
 }

+func TestUnary(t *testing.T) {
+	q := "-up"


worth adding +up case? Can probably generalize this to be like

for _, q := range []string{"+up","-up"} { .... }

+ skips the unary op, so its just a fetch. Will add a test though.

arnikola · 2019-05-22T19:10:59Z

src/query/block/lazy_test.go

+	ctrl := gomock.NewController(t)
+	bb := NewMockBlock(ctrl)
+	defer ctrl.Finish()
+	offset := time.Duration(0)


mind adding one with both transform and offset?

arnikola · 2019-05-22T19:11:27Z

src/query/block/lazy_test.go

+	defer ctrl.Finish()
+	b := NewMockBlock(ctrl)
+	offset := time.Duration(0)
+	off := NewLazyBlock(b, testLazyOpts(offset, -1.0))


nit: can do testLazyOpts(0, -1.0) and it'll typecast it properly

yeah just figured I'd be more explicit with it being a duration.

arnikola

LGTM; some suggestions for additional tests but up to you

prateek · 2019-05-23T17:27:28Z

src/query/block/lazy.go

+		stepVals = c.Values()
+	)
+
+	vals := make([]float64, 0, len(stepVals))


longer term, instead of allocating for each Current() call, you can reuse a field in the struct. it'd look something like this:

type lazyStepIter struct { it StepIter opts LazyOptions values []float64 } func (it *lazyStepIter) Current() Step { var ( c = it.it.Current() tt, vt = it.opts.TimeTransform(), it.opts.ValueTransform() stepVals = c.Values() ) it.values = it.values[:0] if cap(it.values) < len(stepVals) { it.values = make([]float64, 0, len(stepVals)) } ... return ColStep{ ... values: it.values, } }

note: this might cause issues if the callers of the this function are relying on the returned value from this function to be valid after multiple calls to Next()/Current(). Dunno how it's used so can't say for sure. Maybe file an issue to look into this

If we do it this way, would make sense to move all this logic into the Next function so we don't have to recalculate 👍

Yeah, Next() would definitely be the better place for it.

prateek

LGTM

benraskin92 marked this pull request as ready for review May 20, 2019 17:15

benraskin92 changed the title ~~[WIP] [query] Add support for unary expressions~~ [query] Add support for unary expressions May 20, 2019

benraskin92 requested review from arnikola, andrewmains12 and robskillington May 20, 2019 19:20

prateek reviewed May 20, 2019

View reviewed changes

arnikola reviewed May 21, 2019

View reviewed changes

benraskin92 force-pushed the braskin/unary branch from e8b82ba to 3dac576 Compare May 22, 2019 14:13

arnikola reviewed May 22, 2019

View reviewed changes

arnikola approved these changes May 22, 2019

View reviewed changes

prateek reviewed May 23, 2019

View reviewed changes

prateek approved these changes May 23, 2019

View reviewed changes

Benjamin Raskin added 9 commits May 24, 2019 10:00

Add support for unary expressions

25d0a03

Add tests

9487b5e

newline

7a8eb45

Spacing

dca4305

Change comment

ce34318

Add comments

c6e4047

Address comments

12f9496

Address comments

4484c92

Add one more test for +up

4d90fd3

benraskin92 force-pushed the braskin/unary branch from f4206d3 to 4d90fd3 Compare May 24, 2019 15:00

Merge branch 'master' into braskin/unary

d82e662

benraskin92 merged commit 7c01a13 into master May 24, 2019

benraskin92 deleted the braskin/unary branch May 24, 2019 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[query] Add support for unary expressions #1647

[query] Add support for unary expressions #1647

benraskin92 commented May 20, 2019 •

edited

Loading

codecov bot commented May 20, 2019

codecov bot commented May 20, 2019 •

edited

Loading

prateek May 20, 2019

benraskin92 May 20, 2019

prateek May 20, 2019

benraskin92 May 21, 2019

prateek May 20, 2019

prateek May 20, 2019

arnikola May 21, 2019

arnikola May 21, 2019

arnikola May 21, 2019

benraskin92 May 22, 2019

arnikola May 21, 2019

arnikola May 21, 2019

arnikola May 21, 2019 •

edited by benraskin92

Loading

arnikola May 21, 2019

arnikola May 22, 2019

arnikola May 22, 2019

benraskin92 May 22, 2019

arnikola May 22, 2019

arnikola May 22, 2019

benraskin92 May 22, 2019

arnikola left a comment

prateek May 23, 2019

arnikola May 23, 2019

prateek May 23, 2019

prateek left a comment

[query] Add support for unary expressions #1647

[query] Add support for unary expressions #1647

Conversation

benraskin92 commented May 20, 2019 • edited Loading

codecov bot commented May 20, 2019

Codecov Report

codecov bot commented May 20, 2019 • edited Loading

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

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

arnikola May 21, 2019 • edited by benraskin92 Loading

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

arnikola left a 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

prateek left a comment

Choose a reason for hiding this comment

benraskin92 commented May 20, 2019 •

edited

Loading

codecov bot commented May 20, 2019 •

edited

Loading

arnikola May 21, 2019 •

edited by benraskin92

Loading