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

Add fill(linear) to query language #7403

Merged
merged 1 commit into from
Oct 5, 2016
Merged

Add fill(linear) to query language #7403

merged 1 commit into from
Oct 5, 2016

Conversation

desa
Copy link
Contributor

@desa desa commented Oct 3, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
Required only if applicable

You can erase any checkboxes below this note if they are not applicable to your Pull Request.

  • InfluxQL Spec updated
  • Provide example syntax
  • Update man page when modifying a command

This PR introduces fill(linear) #3273 which will fill empty group by intervals linearly between successive values.

There are two interesting cases.

1 missing point

Suppose that I wrote the following points into the database

cpu,host=A value=2 10s
cpu,host=A value=4 30s

Then the query SELECT mean(value) FROM cpu WHERE time >= '1970-01-01T00:00:00Z' AND time <= '1970-01-01T00:01:00Z' GROUP BY time(10s) fill(linear) should return

name: cpu
time    mean
----    ----
0
10  2
20  3
30  4
40
50

> 1 missing point

Suppose I insert the following points

cpu,host=A value=2 10s
cpu,host=A value=7 60s

Then the query SELECT mean(value) FROM cpu WHERE time >= '1970-01-01T00:00:00Z' AND time <= '1970-01-01T00:01:00Z' GROUP BY time(10s) fill(linear) should return

name: cpu
time    mean
----    ----
0
10  2
20  3
30  4
40  5
50  6
60  7

@desa desa force-pushed the md-fill-average branch from f96db74 to cd5fc82 Compare October 3, 2016 18:36
@jwilder
Copy link
Contributor

jwilder commented Oct 3, 2016

@jsternberg

@@ -0,0 +1,20 @@
package influxql

func averageFloat(wt, lt, nt int64, prev, next float64) float64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment describing this and the variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments and changed code format so things should be more clear now. Let me know if theres anything else I can add.

@@ -673,6 +673,23 @@ func (itr *{{$k.name}}FillIterator) Next() (*{{$k.Name}}Point, error) {
switch itr.opt.Fill {
case NullFill:
p.Nil = true
case AverageFill:
Copy link
Contributor

Choose a reason for hiding this comment

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

A less conventional way to do this, but probably better, is to just move AverageFill above NullFill and do this with the condition.

{{if or (eq $k.Name "Float") (eq $k.Name "Integer")}}
... code that you have ...
{{else}}
fallthrough
{{end}}

That way you don't need averageString or averageBoolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be addressed now

@desa
Copy link
Contributor Author

desa commented Oct 3, 2016

When adding comments to this file, it became clear that this should be called fill(linear) not fill(average) since what is being done here is really linear interpolation.

@jsternberg
Copy link
Contributor

jsternberg commented Oct 3, 2016

Haha, that's why I asked. It looked like it was linear interpolation and I was confused because the code was hard to understand. Now, I don't expect math code to be easy to understand, but I did want a comment so I don't have to try reading math 😄

@desa desa changed the title Add fill(average) to query language Add fill(linear) to query language Oct 3, 2016
@desa
Copy link
Contributor Author

desa commented Oct 3, 2016

:)

Let me know if theres anything that I missed.

// and returns the value of the point on the line with time windowTime
// y = mx + b
func linearInteger(windowTime, previousTime, nextTime int64, previousValue, nextValue int64) int64 {
m := (nextValue - previousValue) / (nextTime - previousTime) // the slope of the line
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me like this won't give the most accurate answer. Maybe we should use a float as the slope and then round after we get the result instead of making this into an integer. For example, take the following values:

previousTime: 0
previousValue: 1
nextTime: 10
nextValue: 5
windowTime: 5

Now I would say that the value at 5 should be 3 here because the value halfway in between 1 and 5 is 3. But if I perform the math here, it seems I get a slope of zero and so I get a value of 1. If I use floats, I get the value 3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup. I'll fix that now.

@@ -1133,6 +1133,62 @@ func TestSelect_Fill_Previous_Float(t *testing.T) {
}
}

// Ensure a SELECT query with a fill(linear) statement can be executed.
func TestSelect_Fill_Linear_Float_One(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you include some tests with integers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. This actually caught an issue

@desa
Copy link
Contributor Author

desa commented Oct 4, 2016

@jsternberg Should be good to go. Let me know if theres anything I missed.

Copy link
Contributor

@jsternberg jsternberg left a comment

Choose a reason for hiding this comment

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

Approved, but please squash the commits before merging.

Copy link
Contributor

@jwilder jwilder left a comment

Choose a reason for hiding this comment

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

This needs a changelog entry under features.

Clean up template for fill average

Change fill(average) to fill(linear)

Update average to linear in infuxql spec

Add Integer Tests and associated fixes

Update CHANGELOG for fill(linear)
@desa desa force-pushed the md-fill-average branch from b346221 to 966e550 Compare October 4, 2016 21:27
@desa
Copy link
Contributor Author

desa commented Oct 4, 2016

@jwilder changelog updated.

@desa desa merged commit fc57c0f into master Oct 5, 2016
@desa desa deleted the md-fill-average branch October 5, 2016 17:40
@timhallinflux timhallinflux added this to the 1.1.0 milestone Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants