-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[FIX] PromQL: support variable scalar parameter in aggregations in range queries #16404
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
CC: @beorn7 @charleskorn |
Thanks for tackling this. While this isn't directly un-doing #13744, it neither does what we sketched out in the dev-summit (find out if the parameter is constant at parse-time and take different code paths then). But maybe the approach here is still fast enough? Have you tried running the micro benchmarks? You could compare the effect of this PR with the findings in #13744 . Maybe we could also do something more in the I would also welcome @bboreham and @machine424 to take a look here. You are way better domain experts here than me. |
Remember to run the |
These are the benchmarks run against this PR. First 1000 steps and then 100 steps.
|
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.
Benchmarks look good.
I had another idea about how to make this a bit more elegant (hopefully). See comments.
Looking forward to what @machine424 thinks about this.
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
d5f0fc3
to
8c35138
Compare
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
@beorn7, I've restructured the code for param handling with 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.
thanks, didn't go through all the changes but lgtm overall.
I have a question though.
@machine424, are there any other changes needed besides the TODO you mentioned? |
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, thanks
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.
Thank you very much, both @NeerajGartia21 and @machine424 . And apologies for the long wait. I only have a tiny style nit and a more serious question about the Next
method.
Signed-off-by: Neeraj Gartia <neerajgartia211002@gmail.com>
@beorn7 , I've added your suggestions. Please have another look. Thanks! |
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.
OK, let's do it. I hope the actual PromQL experts will not be upset (but they had plenty of time to chime in).
Also, @machine424 approved it. I'd consider him expert enough. :) |
Fixes #15971