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.

Sign up for GitHub

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

Jump to bottom

Implement derivatives across intervals for aggregate queries #6103

Merged
merged 1 commit into from
Apr 16, 2016
Merged

Implement derivatives across intervals for aggregate queries #6103

merged 1 commit into from
Apr 16, 2016

Conversation

jsternberg
Copy link
Contributor

For aggregate queries, derivatives will now alter the start time to one
interval behind and will use that interval to find the derivative of the
first point instead of giving no value for that interval.

This does not apply to raw queries yet.

Fixes #3247. Contributes to #5943.

@jsternberg
Copy link
Contributor Author

@benbjohnson @jwilder

Also @nathanielc since this changes a public API that may be used by Kapacitor.

@nathanielc
Copy link
Contributor

@jsternberg The API change is OK with me.

@benbjohnson
Copy link
Contributor

@jsternberg Overall, lgtm. Can youc comment the allowNil in a little more detail? I think I understand it but I just want to see it documented for the future.

@jsternberg jsternberg force-pushed the js-3247-derivative-across-intervals branch 2 times, most recently from 67be74a to 5e27f27 Compare March 23, 2016 18:57
@jsternberg jsternberg force-pushed the js-3247-derivative-across-intervals branch from 5e27f27 to 338e9a5 Compare April 4, 2016 19:47
@jsternberg
Copy link
Contributor Author

I removed the allowNil section and modified derivative and difference to use the stream iterators that were used for moving average. I also got rid of the allowNil functionality and nil values, even if they are before the current interval, will just be discarded the same as if they were in the middle or the end.

This should now work for all aggregate queries.

@jsternberg jsternberg added this to the 0.13.0 milestone Apr 4, 2016
@sharang
Copy link

sharang commented Apr 6, 2016

@jsternberg will you merge it into 0.12?

@jsternberg
Copy link
Contributor Author

@sharang unfortunately 0.12 has already been released and this wasn't ready in time to make feature cutoff. It's currently awaiting review and will likely go into 0.13 when we release that.

@jsternberg jsternberg force-pushed the js-3247-derivative-across-intervals branch 2 times, most recently from a94a45a to bd3deeb Compare April 11, 2016 20:35
@toddboom
Copy link
Contributor

@jsternberg is anything else needed on this, or are you just waiting for some upvotes?

@jsternberg
Copy link
Contributor Author

Just waiting. This should work for aggregate queries which should alleviate at least some of the pain from this.

@jsternberg jsternberg force-pushed the js-3247-derivative-across-intervals branch from bd3deeb to ddeedc7 Compare April 13, 2016 17:10
@toddboom
Copy link
Contributor

this seems good to me, though i'd like @benbjohnson to get some eyes on it since i'm not the most capable assessor of query engine changes

@toddboom
Copy link
Contributor

oh, ben already looked at it. i guess it's good to merge? 👍

@jsternberg
Copy link
Contributor Author

It could use another look. @benbjohnson reviewed an older version of this.

@jsternberg jsternberg force-pushed the js-3247-derivative-across-intervals branch from ddeedc7 to 130bc25 Compare April 15, 2016 21:42
@benbjohnson
Copy link
Contributor

👍

@jsternberg
Implement derivatives across intervals for aggregate queries
86046bb 
For aggregate queries, derivatives will now alter the start time to one
interval behind and will use that interval to find the derivative of the
first point instead of giving no value for that interval. Null values
will still be discarded so if the interval before the one you are
querying is null, then it will be discarded like if it were in the
middle of the query. You can use `fill(0)` to fill in these values.

This does not apply to raw queries yet.

Also modified the derivative and difference aggregates to use the stream
iterator instead of the reduce slice iterator for space efficiency.

Fixes #3247. Contributes to #5943.
@jsternberg jsternberg force-pushed the js-3247-derivative-across-intervals branch from 130bc25 to 86046bb Compare April 15, 2016 22:16
@jsternberg jsternberg merged commit 8ed877a into master Apr 16, 2016
@jsternberg jsternberg deleted the js-3247-derivative-across-intervals branch April 16, 2016 13:39
@onnos onnos mentioned this pull request Jun 5, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jsternberg @nathanielc @benbjohnson @sharang @toddboom