Add scalar block and binary functions #822

arnikola · 2018-08-04T13:28:13Z

Added scalar block type for parsing prom constant values

Arithmetic functions added (+, -, *, /, ^, %) for scalar-scalar,
scalar-series, series-scalar, series-series left and right node values

Comparison functions added (==, !=, >, <, >=, <=) for scalar-scalar,
scalar-series, series-scalar, series-series left and right node values �

Added unit tests for all permutations of comparison and arithmetic functions

Still TODO: consolidate binary node with logical node, as a lot of behaviour
is shared.

TODOs: - comparison functions - matching for non-scalar lhs and rhs sides - unit tests - consolidate with logical funcs

nikunjgit · 2018-08-05T22:52:39Z

src/coordinator/block/scalar_block.go

@@ -0,0 +1,150 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


call it scalar.go since its already in block package?

nikunjgit · 2018-08-05T22:54:00Z

src/coordinator/block/scalar_block.go

+	}, nil
+}
+
+func (b *scalarBlock) SeriesIter() (SeriesIter, error) { return nil, nil }


dont you want to return scalarSeriesIter?

nikunjgit · 2018-08-05T22:55:19Z

src/coordinator/block/scalar_block.go

+// StepIter returns a StepIterator
+func (b *scalarBlock) StepIter() (StepIter, error) {
+	bounds := b.meta.Bounds
+	steps := bounds.End.Sub(bounds.Start) / bounds.StepSize


can you define Steps() on Bounds struct itself ? A multiple places in code will need it

Turns out Bounds.Steps() exists already :) using that instead

nikunjgit · 2018-08-05T23:00:07Z

src/coordinator/functions/binary/binary_node.go

@@ -0,0 +1,232 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


codecov · 2018-08-14T19:56:48Z

Codecov Report

Merging #822 into master will decrease coverage by 0.09%.
The diff coverage is 77.41%.

@@            Coverage Diff            @@
##           master     #822     +/-   ##
=========================================
- Coverage   78.42%   78.33%   -0.1%     
=========================================
  Files         375      379      +4     
  Lines       32247    32595    +348     
=========================================
+ Hits        25289    25532    +243     
- Misses       5239     5319     +80     
- Partials     1719     1744     +25

Flag	Coverage Δ
#dbnode	`81.34% <ø> (-0.12%)`	⬇️
#m3ninx	`71.99% <ø> (ø)`	⬆️
#query	`66.34% <77.41%> (+0.86%)`	⬆️

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 1eb2dd7...d3640fe. Read the comment docs.

nikunjgit · 2018-08-15T18:17:00Z

src/query/block/scalar.go

+
+// ScalarBlock is a block containing a single value
+// over a certain bound
+type ScalarBlock struct {


call it Scalar since its already in block package. Also do you really need to expose it outside ? Seems like you outside functions should just work on the Block interface

I cast to Scalar block sometimes as an optimization to pull out the value for the entire block, rather than having to step through each value one by one. Will rename

nikunjgit · 2018-08-15T18:18:08Z

src/query/block/scalar.go

+
+// NewScalarBlockResult creates a Result consisting
+// of scalar blocks over the bound
+func NewScalarBlockResult(val float64, bounds Bounds) *Result {


why does result need to know about scalar block ?

I was mirroring how fetch performed executions in NewScalarOp; changed it a bit to only build a single block and use that instead.

nikunjgit · 2018-08-15T18:20:24Z

src/query/block/scalar.go

+	"github.com/m3db/m3/src/query/models"
+)
+
+// ScalarBlock is a block containing a single value


can you add a comment as to when this would be useful ?

nikunjgit · 2018-08-15T18:25:04Z

src/query/block/scalar.go

+}
+
+func (it *scalarStepIter) Current() (Step, error) {
+	if it.idx >= it.numVals || it.idx < 0 {


we haven't traditionally been checking this in Current() usually rely on people to call Next() before Current

I agree in general, but since this doesn't do any real "bounds" checking and just returns a value, the alternative is that this panics in this check, as it really shouldn't be returning anything if Next() is false.

I prefer to error than to panic here, but if you have particular preference towards panicing I can change to that

other places have panics instead of errors since its a way to avoid more codepaths :( I guess we can do either here since its returning error but a lot of times Current() only returns data and panic is the only option.

Yeah, if it weren't already able to return an error I would've paniced, but figured might as well make use of the capability here so we don't kill the node :)

nikunjgit · 2018-08-15T18:25:48Z

src/query/block/scalar.go

+}
+
+func (it *scalarSeriesIter) Current() (Series, error) {
+	if it.idx != 0 {


we haven't traditionally been checking this in Current() usually rely on people to call Next() before Current

I agree in general, but since this doesn't do any real "bounds" checking and just returns a value, the alternative is that this panics in this check, as it really shouldn't be returning anything if Next() is false.

I prefer to error than to panic here, but if you have particular preference towards panicing I can change to that

same reply as above

nikunjgit · 2018-08-15T18:48:15Z

src/query/functions/binary/node.go

+}
+
+// computeOrCache figures out if both lhs and rhs are available, if not then it caches the incoming block
+func (n *binaryNode) computeOrCache(ID parser.NodeID, b block.Block) (block.Block, block.Block, error) {


is this the same as the other place ? If yes, then think about refactor

Yeah, this falls under:
TODO: consolidate binary node with logical node, as a lot of behaviour
is shared.

Might do that in a separate commit once this is in. No other binary functions are planned in the near future so shouldn't affect any upcoming commits

cool, was just checking

nikunjgit · 2018-08-15T18:48:34Z

src/query/functions/binary/node.go

+
+// intersect returns the slice of lhs indices that are shared with rhs,
+// the indices of the corresponding rhs values, and the metas for taken indices
+func intersect(


is this the same as the other place ? If yes, then think about refactor

It's slightly different in that this one is designed to be able to drop series which do not match, consistent with prom behaviour here.

nikunjgit · 2018-08-15T18:49:44Z

src/query/functions/binary/node.go

+
+// If returnBool is false and op is a comparison function,
+// the function should return the scalar value rather than 1 or 0
+func actualScalarFunc(fn singleScalarFunc, op string, returnBool bool) singleScalarFunc {


nikunjgit · 2018-08-15T18:51:37Z

src/query/functions/binary/node.go

+// If returnBool is false and op is a comparison function,
+// the function should return the scalar value of lhs if the comparison
+// is true; otherwise, it should return 1 if true, and 0 if false
+func actualFunc(fn mathFunc, op string, returnBool bool) mathFunc {


nikunjgit · 2018-08-15T18:53:47Z

src/query/functions/binary/node.go

+	}
+
+	if info.RIsScalar {
+		scalarR, ok := rhs.(*block.ScalarBlock)


some duplication here:

maybe create a function which takes two block.Block, assumes first can be casted into scalar and other can't be. Then you can use the same function for this case and one of the previous cases

I'm not sure that would reduce duplication much, and may make it a bit more confusing since I'd need to use some kind of scalarLeft bool; it also introduces a couple of extra bool checks.

All I'd be saving by using a function for this would be the cast to *block.ScalarBlock, since the singleScalarFunction would be different for either branch

benraskin92 · 2018-08-16T19:03:02Z

src/query/block/scalar.go

+)
+
+// Scalar is a block containing a single value
+// over a certain bound


nit: add this to end of first line

benraskin92 · 2018-08-16T19:10:16Z

src/query/block/scalar.go

+type scalarStepIter struct {
+	meta    Metadata
+	step    scalarStep
+	numVals int


nit: numVals, idx int

Looks a bit uglier but ok :p

benraskin92 · 2018-08-16T19:13:49Z

src/query/block/scalar.go

+var _ SeriesIter = (*scalarSeriesIter)(nil)
+
+func (it *scalarSeriesIter) Close()                   { /* No-op*/ }
+func (it *scalarSeriesIter) SeriesCount() int         { return 1 }


This is kind of weird- wouldn't this always be 1 in all SeriesIters?

If a SeriesIter is created from a block with multiple series, SeriesCount would be the number of the series in that block; in the case of a scalar block, there's only one "series" which has val as the value at each step.

benraskin92 · 2018-08-16T19:17:23Z

src/query/block/scalar_test.go

+
+var (
+	start  = time.Time{}
+	val    = 13.37


Can we make this like 100 or something simple?

Wanted to make it explicitly a float with decimal places

math.Pi is also an option

benraskin92 · 2018-08-16T19:35:01Z

src/query/functions/binary/node.go

+
+const (
+	// *******************************************
+	// * Arithmetic functions


What's going on with the *s?

Just visual delineation

benraskin92 · 2018-08-16T19:44:26Z

src/query/functions/binary/node.go

+			return n.processSingleBlock(
+				rhs,
+				func(x float64) float64 {
+					return n.op.fn(lVal, x)


Can you add a comment here?

benraskin92 · 2018-08-16T19:48:44Z

src/query/functions/binary/node.go

+	lIter, rIter block.StepIter,
+) (block.Block, error) {
+	lMeta, rMeta := lIter.Meta(), rIter.Meta()
+


Check to make sure bounds are equal

it we're doing the check, should that be in process() ?

benraskin92 · 2018-08-16T20:14:03Z

src/query/functions/binary/node.go

+			return nil, err
+		}
+
+		lValues := lStep.Values()


Get the values right before you use them below instead of here

benraskin92 · 2018-08-16T20:14:12Z

src/query/functions/binary/node.go

+			return nil, err
+		}
+
+		rValues := rStep.Values()


This one's already right before it's used

Oh yeah lol

benraskin92 · 2018-08-16T20:31:46Z

src/query/parser/promql/parse.go

@@ -176,6 +179,18 @@ func (p *parseState) walk(node pql.Node) error {
 		p.transforms = append(p.transforms, opTransform)
 		return nil

+	case *pql.NumberLiteral:
+		op := NewScalarOperator(n)
+


nit: remove newline

benraskin92 · 2018-08-16T20:32:19Z

src/query/parser/promql/parse_test.go

-func TestDAGWithAbsentOp(t *testing.T) {
-	q := "absent(http_requests_total{method=\"GET\"})"
-	p, err := Parse(q)
+var linearParseTests = []struct {


benraskin92 · 2018-08-16T20:34:08Z

src/query/parser/promql/types.go

@@ -178,6 +239,10 @@ func promVectorCardinalityToM3(card promql.VectorMatchCardinality) logical.Vecto
 }

 func promMatchingToM3(vectorMatching *promql.VectorMatching) *logical.VectorMatching {
+	// vectorMatching can be nil iff at least one of the sides is a scalar


nit: s/iff/if

It's intentional

nikunjgit · 2018-08-16T20:27:31Z

src/query/block/scalar.go

+}
+
+// assert that Scalar implements Block
+var _ Block = (*Scalar)(nil)


you don't need it since NewScalar returns Block

nikunjgit · 2018-08-16T20:30:33Z

src/query/block/scalar.go

+}
+
+// assert that scalarStepIter implements StepIter
+var _ StepIter = (*scalarStepIter)(nil)


you don't need it since StepIter() returns & scalarStepIter

nikunjgit · 2018-08-16T20:31:59Z

src/query/block/scalar.go

+}
+
+func (it *scalarStepIter) Current() (Step, error) {
+	if it.idx >= it.numVals || it.idx < 0 {


other places have panics instead of errors since its a way to avoid more codepaths :( I guess we can do either here since its returning error but a lot of times Current() only returns data and panic is the only option.

nikunjgit · 2018-08-16T20:32:35Z

src/query/block/scalar.go

+}
+
+// assert that scalarStepIter implements StepIter
+var _ Step = (*scalarStep)(nil)


don't think you need this

nikunjgit · 2018-08-16T20:32:47Z

src/query/block/scalar.go

+}
+
+// assert that scalarStepIter implements StepIter
+var _ SeriesIter = (*scalarSeriesIter)(nil)


same as above

nikunjgit · 2018-08-16T20:35:14Z

src/query/executor/state.go

+	options transform.Options,
+) (parser.Source, *transform.Controller) {
+	controller := &transform.Controller{ID: ID}
+


extra line ?

nikunjgit · 2018-08-16T20:35:30Z

src/query/executor/transform/types.go

@@ -47,6 +47,15 @@ type TimeSpec struct {
 	Step time.Duration
 }

+// ToBounds transforms a timespec to bounds
+func (ts TimeSpec) ToBounds() block.Bounds {


nikunjgit · 2018-08-16T20:41:57Z

src/query/functions/binary/node.go

+	lIter, rIter block.StepIter,
+) (block.Block, error) {
+	lMeta, rMeta := lIter.Meta(), rIter.Meta()
+


it we're doing the check, should that be in process() ?

nikunjgit · 2018-08-16T20:42:34Z

src/query/functions/binary/node.go

+}
+
+// computeOrCache figures out if both lhs and rhs are available, if not then it caches the incoming block
+func (n *binaryNode) computeOrCache(ID parser.NodeID, b block.Block) (block.Block, block.Block, error) {


cool, was just checking

nikunjgit · 2018-08-16T20:43:17Z

src/query/functions/binary/node.go

+	n.cache.Remove(n.op.params.RNode)
+}
+
+const initIndexSliceLength = 10


makes sense to move it to the top ? We might want to put some of these things in the config later.

arnikola added 18 commits July 13, 2018 17:38

Add logical OR function

21a89d3

Add LOR type to parser

cf82fcf

Fixed functions post rebase

358d1df

Fixed test

d69abe0

Updated with comments, add logical unless function

230e07a

Merge branch 'master' into arnikola/orOp

7a49048

Fixed some logical inconstistencies with Prometheus functions

5644434

Metalint fix

5bcc650

Merge branch 'master' into arnikola/orOp

9b8a501

Refactor logical functions

8d2a2a9

Refactor tests

a6f80cb

Additional tests

edf3b84

lint

3fff221

PR response

b1e1413

add combineSeriesMeta function

6ba57af

Combining series specific metadata as well as shared block metadata

7fa3208

Extract common tags before doing logical comparisons

f53c81c

Add scalar block and binary functions

44ac09c

TODOs: - comparison functions - matching for non-scalar lhs and rhs sides - unit tests - consolidate with logical funcs

nikunjgit reviewed Aug 5, 2018

View reviewed changes

arnikola added 6 commits August 6, 2018 19:37

PR response, adding parse and scalar block tests

6b68e65

scalar test changes

d2b8b33

Add binary node tests

f836954

Add series/series scalar functions and test

216b8bb

Tests for arithmetic and comparisons

00b5f17

Merge branch 'master' into arnikola/binOps

e6dd256

arnikola changed the base branch from arnikola/orOp to master August 14, 2018 19:45

arnikola requested a review from benraskin92 August 15, 2018 18:21

nikunjgit reviewed Aug 15, 2018

View reviewed changes

PR responses

d97335d

benraskin92 reviewed Aug 16, 2018

View reviewed changes

nikunjgit approved these changes Aug 16, 2018

View reviewed changes

arnikola added 2 commits August 16, 2018 17:27

PR response

d3465bb

Merge branch 'master' into arnikola/binOps

d3640fe

arnikola merged commit b63563e into master Aug 16, 2018

arnikola deleted the arnikola/binOps branch August 16, 2018 22:33

		@@ -0,0 +1,150 @@
		// Copyright (c) 2018 Uber Technologies, Inc.

		@@ -0,0 +1,232 @@
		// Copyright (c) 2018 Uber Technologies, Inc.

Add scalar block and binary functions #822

Add scalar block and binary functions #822

Conversation

arnikola commented Aug 4, 2018 • edited 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

codecov bot commented Aug 14, 2018 • 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

arnikola Aug 15, 2018 • edited 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

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

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

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

arnikola commented Aug 4, 2018 •

edited

Loading

codecov bot commented Aug 14, 2018 •

edited

Loading

arnikola Aug 15, 2018 •

edited

Loading