Add temporal functions #872

benraskin92 · 2018-08-30T19:06:54Z

No description provided.

benraskin92 · 2018-08-30T19:08:22Z

src/query/functions/temporal/aggregation.go

+		return aux / count
+
+	default:
+		panic("unknown aggregation type")


This doesn't seem right, but, on the other hand, if we get to this point, something is seriously wrong.

Rather than doing this, you can have
type aggFunc func(vals float64) float64
Then have these all be seperate functions, and have aggFuncs be a map of these, similar to how aggregation functions are done in aggregation/base.go

Yeah, I thought it'd be a little messier, but on second thought, this is probably better. Will make the change.

👍
Another nice thing you get is that you don't need to check opType twice

arnikola · 2018-08-30T19:31:24Z

src/query/functions/temporal/aggregation.go

+		return aux / count
+
+	default:
+		panic("unknown aggregation type")


Rather than doing this, you can have
type aggFunc func(vals float64) float64
Then have these all be seperate functions, and have aggFuncs be a map of these, similar to how aggregation functions are done in aggregation/base.go

arnikola · 2018-08-30T19:32:22Z

src/query/parser/promql/parse_test.go

-	assert.Len(t, edges, 1)
-	assert.Equal(t, edges[0].ParentID, parser.NodeID("0"), "fetch should be the parent")
-	assert.Equal(t, edges[0].ChildID, parser.NodeID("1"), "aggregation should be the child")
+var temporalParseTests = []struct {


arnikola · 2018-08-30T19:34:22Z

src/query/functions/temporal/aggregation.go

+				min = math.Min(min, v)
+			}
+		}
+		return min


This fails edge cases; if all values are NaNs, this will return math.Inf(1) instead of math.NaN()

arnikola · 2018-08-30T19:34:44Z

src/query/functions/temporal/aggregation.go

+				max = math.Max(max, v)
+			}
+		}
+		return max


This fails edge cases; if all values are NaNs, this will return math.Inf(-1) instead of math.NaN()

arnikola · 2018-08-30T19:36:36Z

src/query/functions/temporal/aggregation.go

+		for _, v := range values {
+			if !math.IsNaN(v) {
+				count++
+				mean += (v - mean) / count


Why do you need a rolling average here? Can't you just keep track of sum and count then return sum/count? You'll also need to account for cases where count == 0

Was following how Prom did it, but yeah we can just do that instead.

arnikola · 2018-08-30T19:38:56Z

src/query/functions/temporal/aggregation.go

+		}
+		return sum
+
+	case StdDevTemporalType:


This is calculating population standard deviation rather than standard deviation, is that what prom uses?

Also will need to have special cases for count == 0 so you don't get div by zeroes if all values are NaN

Yeah that's what Prom uses.

arnikola · 2018-08-30T19:39:09Z

src/query/functions/temporal/aggregation.go

+				aux += delta * (v - mean)
+			}
+		}
+		return aux / count


Same here re: population standard variance

arnikola · 2018-08-30T19:45:15Z

src/query/functions/temporal/aggregation_test.go

+// B1 has NaN in first series, first position
+func TestAggregation(t *testing.T) {
+	for _, testCase := range testCases {
+		values, bounds := test.GenerateValuesAndBounds(nil, nil)


Unfortunately if you're using this as your values, you only get a really nice set of data points which do not help catch weird edge case issues

Will add other test cases

Actually can we remove this GenerateValues thing, it leads to really confusing tests and it's honestly a bit weird that we generate test data somewhere different from the test itself :\

Yeah- I can do that in another pr

arnikola · 2018-08-30T19:46:26Z

src/query/functions/temporal/aggregation_test.go

+
+// B1 has NaN in first series, first position
+func TestAggregation(t *testing.T) {
+	for _, testCase := range testCases {


The typical way of running these tabled tests is:

for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { ...

arnikola · 2018-08-30T19:48:02Z

src/query/functions/temporal/aggregation.go

+)
+
+// NewAggOp creates a new base temporal transform with a specified node
+func NewAggOp(args []interface{}, optype string) (transform.Params, error) {


I'd recommend just taking floats or whatever as your args, and have the logic for providing usable values in the parser instead, so you don't end up with extra parsing scattered around in the function op.

i think the problem here is that some ops require more than one args. Then the question is where should that logic live which determines which argument is need by which function.

We should add a todo to consolidate how we pass args around, #884 should help us standardize scalar parameters, but unfortunately there's a few special cases in prom which take Scalar, Fetch instead of Fetch, Scalar, or even Fetch, Scalar, Scalar. Prom have a set of required param types by position; may be an idea to do something similar?

arnikola · 2018-09-04T14:19:44Z

src/query/functions/temporal/aggregation_test.go

+			{math.NaN(), math.NaN(), math.NaN(), math.NaN(), 1.4142},
+		},
+		afterAllBlocks: [][]float64{
+			{1.4142, 1.4142, 1.4142, 1.4142, 1.4142},


This doesn't seem right, aren't there two NaNs in the first series? How does that give the same expected as the other series?

pls respond

Just one NaN, but it gets converted back to the original value (0) on line 270.

values[0][0] = original

codecov · 2018-09-04T19:08:38Z

Codecov Report

Merging #872 into master will increase coverage by 22.76%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master     #872       +/-   ##
===========================================
+ Coverage    55.8%   78.57%   +22.76%     
===========================================
  Files         392      396        +4     
  Lines       33328    33549      +221     
===========================================
+ Hits        18599    26360     +7761     
+ Misses      13067     5385     -7682     
- Partials     1662     1804      +142

Flag	Coverage Δ
#dbnode	`81.27% <ø> (+11.63%)`	⬆️
#m3ninx	`72.39% <ø> (+50.76%)`	⬆️
#query	`69.49% <100%> (+61.14%)`	⬆️

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 2cf04be...106cab0. Read the comment docs.

arnikola · 2018-09-04T19:11:34Z

src/query/functions/temporal/aggregation.go

-				min = math.Min(min, v)
-			}
+	}
+	if count == 0 {


nit: newlines

Also consider doing something like aggregation functions do where you have a sumAndCount function which returns (sum, count float64), making a bunch of these easier

arnikola · 2018-09-04T19:12:25Z

src/query/functions/temporal/aggregation.go

-				sum += v
-			}
+	}
+	if min == math.Inf(1) {


newlines

Also this might be a bit dumb, but if math.Inf somehow appears in the series, you'd get incorrect results

arnikola · 2018-09-04T19:16:58Z

src/query/functions/temporal/aggregation_test.go

+				bounds block.Bounds
+			)
+
+			if vals == nil {


Can you just pass in values instead of doing this generation thing? It's really bad for comprehension and understanding why the results are what they are

nikunjgit · 2018-09-04T20:12:05Z

src/query/functions/temporal/aggregation.go

+}
+
+func newAggNode(op baseOp, controller *transform.Controller) Processor {
+	return &aggNode{


maybe just store the function directly so that we don't have to do the lookup in Process ?

nikunjgit · 2018-09-04T20:12:53Z

src/query/functions/temporal/aggregation.go

+	return aggFuncs[a.op.operatorType](values)
+}
+
+func avgOverTime(values []float64) float64 {


all these functions look very similar to what Artem wrote. Consider consolidating them ?

nikunjgit · 2018-09-05T00:26:33Z

src/query/functions/temporal/aggregation.go

+	return sum
+}
+
+func stddevOverTime(values []float64) float64 {


instead of implementing all these functions, can you use : gonum ? Eg: https://github.com/gonum/gonum/blob/73ea1e732937f96d723d31dc5263d214a275d204/stat/stat.go#L1199

arnikola · 2018-09-06T15:06:58Z

src/query/functions/temporal/aggregation.go

+// NewAggOp creates a new base temporal transform with a specified node
+func NewAggOp(args []interface{}, optype string) (transform.Params, error) {
+	if _, ok := aggFuncs[optype]; !ok {
+		return emptyOp, fmt.Errorf("unknown aggregation type: %s", optype)


nit: nil should work instead of emptyOp

arnikola · 2018-09-06T15:08:17Z

src/query/functions/temporal/aggregation.go

+	return &aggNode{
+		op:         op,
+		controller: controller,
+		aggFunc:    aggFuncs[op.operatorType],


You've already retrieved the aggFunction previously, when you were checking for map membership in NewAggOp. Can you just pass that through instead of re-fetching it here?

arnikola · 2018-09-06T15:08:53Z

src/query/functions/temporal/aggregation.go

+type aggNode struct {
+	op         baseOp
+	controller *transform.Controller
+	aggFunc    func([]float64) float64


nit: define this as a type, i.e. type aggFunc func([]float64) float64

arnikola · 2018-09-06T15:09:28Z

src/query/functions/temporal/aggregation.go

+)
+
+var (
+	aggFuncs = map[string]func([]float64) float64{


nit: define and use an aggFunc type here

arnikola · 2018-09-06T15:11:13Z

src/query/functions/temporal/aggregation.go

+	return sum
+}
+
+func stddevOverTime(values []float64) float64 {


If not using gonum, can this just return math.Sqrt(stdvarOverTime(values))

arnikola · 2018-09-06T15:14:33Z

src/query/parser/promql/types.go

@@ -141,8 +141,10 @@ func NewFunctionExpr(name string, argValues []interface{}) (parser.Params, error
 		linear.MinuteType, linear.MonthType, linear.YearType:
 		return linear.NewDateOp(name)

-	case temporal.CountTemporalType:
-		return temporal.NewCountOp(argValues)
+	case temporal.AvgTemporalType, temporal.CountTemporalType, temporal.MinTemporalType,


take it or leave it: All of these are getting pretty long, may be an idea to make it into a function that checks if it's one of these instead? Your call

arnikola · 2018-09-06T15:15:20Z

src/query/functions/temporal/aggregation.go

+			count++
+		}
+	}
+	return count


nit: newline missing in most of these pre-return

arnikola · 2018-09-06T15:17:25Z

src/query/functions/temporal/aggregation.go

+	return a.aggFunc(values)
+}
+
+func avgOverTime(values []float64) float64 {


I liked @nikunjgit's idea in the regular aggregation functions to make a function that returns (sum, count) which simplifies sum, count, average, and can help with stddev/stdvar

arnikola · 2018-09-06T15:17:46Z

src/query/functions/temporal/aggregation.go

+}
+
+func minOverTime(values []float64) float64 {
+	var count float64


This can just be a bool, seenNotNaN or something?

arnikola · 2018-09-06T15:19:46Z

src/query/functions/temporal/aggregation.go

+}
+
+func maxOverTime(values []float64) float64 {
+	var count float64


This can just be a bool, seenNotNaN or something?

arnikola · 2018-09-06T17:38:16Z

src/query/functions/temporal/aggregation.go

-		}
-	}
-
+	sum, count := sumAndCount(values)
 	if count == 0 {
 		return math.NaN()


nit: this logic might be better to put in sumAndCount?

But then you'd still have to have an if statement here, no?

arnikola · 2018-09-06T17:40:57Z

src/query/functions/temporal/aggregation.go

@@ -64,18 +66,23 @@ var (

 // NewAggOp creates a new base temporal transform with a specified node
 func NewAggOp(args []interface{}, optype string) (transform.Params, error) {
-	if _, ok := aggFuncs[optype]; !ok {
-		return emptyOp, fmt.Errorf("unknown aggregation type: %s", optype)
+	var (


nit: would read cleaner as

if aggregationFunc, ok := aggFuncs[optype]; ok { return newBaseOp(args, optype, newAggNode, aggregationFunc) } return nil, fmt.Errorf("unknown aggregation type: %s", optype)

arnikola · 2018-09-06T17:41:40Z

src/query/functions/temporal/aggregation.go

-		}
-	}
-
+	sum, count := sumAndCount(values)
 	if count == 0 {
 		return math.NaN()


nit: this logic might be better to put in sumAndCount?

arnikola · 2018-09-06T21:55:28Z

src/query/functions/temporal/aggregation.go

 	return sum / count
 }

 func countOverTime(values []float64) float64 {
 	_, count := sumAndCount(values)
+	if count == 0 {
+		return math.NaN()
+	}
 	return count


nit: newline

benraskin92 commented Aug 30, 2018

View reviewed changes

benraskin92 requested review from nikunjgit and arnikola August 30, 2018 19:15

arnikola reviewed Aug 30, 2018

View reviewed changes

arnikola reviewed Sep 4, 2018

View reviewed changes

nikunjgit reviewed Sep 4, 2018

View reviewed changes

nikunjgit reviewed Sep 5, 2018

View reviewed changes

arnikola reviewed Sep 6, 2018

View reviewed changes

arnikola closed this Sep 6, 2018

arnikola deleted the braskin/time_funcs branch September 6, 2018 17:52

Benjamin Raskin added 3 commits September 6, 2018 14:04

Add aggregation time funcs

a2c030f

Update tests

b384e47

Fix lint error

df23304

Benjamin Raskin added 5 commits September 6, 2018 14:06

Newline

c30b716

Address comments

5c0723d

Address more comments

d0d4b63

More comments

71a0630

Address comments

ae056da

benraskin92 restored the braskin/time_funcs branch September 6, 2018 18:17

benraskin92 reopened this Sep 6, 2018

More comments

629e97e

benraskin92 force-pushed the braskin/time_funcs branch from f123dd9 to 629e97e Compare September 6, 2018 20:17

Update count

7cec474

arnikola reviewed Sep 6, 2018

View reviewed changes

arnikola approved these changes Sep 6, 2018

View reviewed changes

Newline

106cab0

benraskin92 merged commit 27e784d into master Sep 7, 2018

prateek deleted the braskin/time_funcs branch September 29, 2018 18:08

Add temporal functions #872

Add temporal functions #872

Conversation

benraskin92 commented Aug 30, 2018

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

codecov bot commented Sep 4, 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

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

codecov bot commented Sep 4, 2018 •

edited

Loading