-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
@@ -0,0 +1,20 @@ | |||
package influxql | |||
|
|||
func averageFloat(wt, lt, nt int64, prev, next float64) float64 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be addressed now
When adding comments to this file, it became clear that this should be called |
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 😄 |
:) 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
@jsternberg Should be good to go. Let me know if theres anything I missed. |
There was a problem hiding this 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.
There was a problem hiding this 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)
@jwilder changelog updated. |
Required for all non-trivial PRs
Required only if applicable
You can erase any checkboxes below this note if they are not applicable to your Pull Request.
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
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> 1 missing point
Suppose I insert the following points
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