Update Derivative for PositionPoint objects #5000
Conversation
| name: "calculate derivative of count with unit 4s group by time with fill previous", | ||
| command: `SELECT derivative(count(value), 4s) from db0.rp0.cpu where time >= '2010-07-01 18:47:00' and time <= '2010-07-01 18:47:03' group by time(2s) fill(previous)`, | ||
| exp: `{"results":[{"series":[{"name":"cpu","columns":["time","derivative"],"values":[["2010-07-01T18:47:02Z",0]]}]}]}`, | ||
| }, |
There was a problem hiding this comment.
This is the failing count + derivative test.
There was a problem hiding this comment.
With my latest commit this is now passing, by explicitly using fill for count 0 time buckets.
nathanielc
force-pushed
the
nc-issue#4849
branch
from
556564d to
56a36bf
Compare
|
looks good. Count should return 0. I think either fill shouldn't work on count, or fill should fill in any zero values on count. Nil and 0 are the same with a count query. What you think? |
|
I think fill should fill in any zeros on Count. I'll see how easy that is to get working. |
|
I vote that count returns nil (or nothing) like all other aggregates do, and if you want it |
|
Count returning nil was actually a bug that we recently fixed. For other aggregates it makes sense to return nil, but not with count. Count is always defined. If there are no data points, it's zero. |
|
https://github.com/influxdb/influxdb/pull/4778/files was the fix for count that @pauldix mentioned. |
nathanielc
force-pushed
the
nc-issue#4849
branch
2 times, most recently
from
f97e7af to
2011966
Compare
| // They're either filling with previous values or a specific number | ||
| for i, vals := range results { | ||
| // start at 1 because the first value is always time | ||
| for j := 1; j < len(vals); j++ { | ||
| if vals[j] == nil { | ||
| if vals[j] == nil || (isCount && isZero(vals[j])) { | ||
| switch e.stmt.Fill { |
There was a problem hiding this comment.
Here is the logic for filling zero count values.
@corylanou @pauldix Thoughts?
|
Generally looks good. I would like to see a test with |
|
@corylanou not sure I follow which test case you want. Something like this: Fails with an error about the query not containing a field before any of the fill logic can be evaluated. The code doesn't understand triply nested functions as far as I can tell. |
…ons, convert PositionPoint to float64
nathanielc
force-pushed
the
nc-issue#4849
branch
from
2011966 to
7ffbbc1
Compare
| name: "calculate derivative of count of distinct with unit default (4s) group by time with fill previous", | ||
| command: `SELECT derivative(count(distinct(value))) from db0.rp0.position where time >= '2010-07-01 18:47:00' and time <= '2010-07-01 18:47:07' group by time(4s) fill(previous)`, | ||
| exp: `{"results":[{"error":"aggregate call didn't contain a field derivative(count(distinct(value)))"}]}`, | ||
| }, |
There was a problem hiding this comment.
Triple nested function fail check.
|
|
Update Derivative for PositionPoint objects
The selectors that return PositionPoint were broken. Added logic to convert PositionPoint objects to float64. This fixes #4849.
I added integration tests for derivative functions with all other valid functions.
One variation doesn't work and I am curious if my test is correct or not.
A count query with fill previous doesn't work as expected since a group by interval with no points has a well defined count of 0. So the fill logic never sees a nil value to fill in with the previous value.
Give a query:
SELECT derivative(count(value)) from db0.rp0.cpu where time >= '2010-07-01 18:47:00' and time <= '2010-07-01 18:47:03' group by time(2s) fill(previous)With a dataset that is missing a all data points for a 2s interval the count returns 0 so the fill(previous) doesn't work and the derivative goes negative.
@corylanou @pauldix Thoughts on whether count with group by should return 0 or nil?