-
Notifications
You must be signed in to change notification settings - Fork 634
MQE: Add support for more aggregation functions #9008
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
Conversation
b9fefa3
to
6e424ff
Compare
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.
Overall design looks good to me 🚀
Benchmark vs main:
tldr: about a 5% overhead with no pooling of aggregation functions. |
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.
Overall LGTM 🚀
I'd like to see some up-to-date benchmark results with the latest round of changes (especially the fix for emitAnnotationFunc
).
My main concern is the change in 6605803: I don't believe this is correct, or an improvement:
- consider the case where an output group receives a float, then a histogram, then a final float for a single output timestamp: we'll end up returning the final float, but we should return nothing
- we'll also now do a whole lot more work: instead of checking for the possibility of conflicts once per output series, and only checking for conflicts at each point if there are histograms and floats present in a single output series, we'll now do a check for every single input point
This reverts commit 6605803.
Good catch, thanks. For simplicity I will revert this, but I may look at doing something else in a separate PR. This PR should be ready now, so long as we're okay with doing the |
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.
LGTM modulo small suggestions below
What this PR does
max
aggregation functionWhich issue(s) this PR fixes or relates to
Related to #2280
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.