MQE: subqueries #9664
MQE: subqueries #9664
Conversation
…ather than receive them
There was a problem hiding this comment.
Overall looks really good to me, nice work!
I think there's just some minor improvements and extra tests we can do, but otherwise the approach and implementation turned out to be very neat with the structures we had in place :-).
There seems to be limited testing around NH in subqueries. We should add some more to ensure our buffers/reuse etc are correct. In doing so, it would also be nice to see some more nested sub-queries (eg 3+ sub-queries deep). Additionally different offsets or @ etc in the sub-queries would be good to check for correct step alignment.
Lastly, perhaps also a test, or a once off benchmark, for where we check at the extremes of having points at the end of the buffer outside the range (ie to check the previously discussed performance regression).
There are already a bunch of tests that cover offsets, @ and nesting (albeit not three levels deep) in the upstream test cases. Is there something in particular that you'd like to see covered that's not already covered there? |
Not sure I follow this sorry - this case is exercised for every range query with more than one step, and is covered by those benchmarks as well. |
Where the step continues to move from the parent query at each depth level. (even if this doesn't really happen due to aligning with epoch).
I was thinking as a one-off benchmark that does it at an extreme, but you're probably right that this isn't necessary. |
Done, I've added a test like this in 220e5de. |
| eval range from 0 to 4m step 20s sum_over_time(sum_over_time(metric[2m:30s])[3m:15s]) | ||
| {} 0 0 0 1 2 4 10 14 20 35 43 54 78 | ||
|
|
||
| eval range from 0 to 4m step 3m sum_over_time(sum_over_time(sum_over_time(metric[2m:30s])[3m:15s])[4m:20s]) |
* Enable upstream tests * Add benchmark * Introduce feature toggle * Don't assume all operators are running at the top level of a query * Add ability to reuse an existing point slice for a ring buffer * Add `Release` method to ring buffers * Introduce range query tests * Bring in TestSubquerySelector from Prometheus * Change `RangeVectorOperator.NextStepSamples` to return ring buffers rather than receive them * Refactor `TestSubqueries` * Initial (largely working) implementation * Fix handling of @ * Enable newly supported upstream test cases * Add further benchmark * Add changelog entry * Address PR feedback: clarify comments Co-authored-by: Joshua Hesketh <[email protected]> * Add tests for ring buffer `Release` implementations * Address PR feedback: update comment to match new behaviour * Address PR feedback: fix indentation * Expand native histogram tests * Add test for deeply nested subqueries with changing step. * Run test cases in TestSubqueries against Prometheus' engine too. --------- Co-authored-by: Joshua Hesketh <[email protected]> (cherry picked from commit 3031f23) Co-authored-by: Charles Korn <[email protected]>
What this PR does
This PR adds support for subqueries to MQE.
Our benchmarks show mild latency improvements in most cases over Prometheus' engine, and improvements in peak memory utilisation in most cases:
It is possible to improve the performance of the 1000 step cases, however to do this without affecting the performance of range vector selectors requires a bit of refactoring I'd prefer to do in a separate PR.
Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.