MQE: add support for and
#9507
MQE: add support for and
#9507
Conversation
# Conflicts: # cmd/mimir/help-all.txt.tmpl # pkg/streamingpromql/config.go # pkg/streamingpromql/engine_test.go
4266a84 to
2b1c9e9
Compare
2b1c9e9 to
54748d8
Compare
| requireQueryIsUnsupported(t, featureToggles, "metric{} or other_metric{}", "binary expression with 'or'") | ||
| requireQueryIsUnsupported(t, featureToggles, "metric{} unless other_metric{}", "binary expression with 'unless'") | ||
|
|
||
| // Other operations should still be supported. |
There was a problem hiding this comment.
(nit for potential followup) I feel like we could avoid some repetition in these feature flag tests
There was a problem hiding this comment.
Agree, although I expect there shouldn't be any more cases added here in the future, and these feature flags are temporary, so I'm not sure it's worth the effort.
pkg/streamingpromql/operators/vector_vector_binary_operation.go
Outdated
Show resolved
Hide resolved
| a.leftSeriesGroups = a.leftSeriesGroups[1:] | ||
|
|
||
| if thisSeriesGroup == nil { | ||
| // The next series from the left side has no matching series on the right side. |
There was a problem hiding this comment.
This is a bit confusing, because a.Left.NextSeries(ctx) will be the series at the same index as thisSeriesGroup. Really it's "this series", not "the next series".
I suggest to move reading the next series to outside of this check (also then not needing it on line 157). The if condition can still put the series back and continue.
There was a problem hiding this comment.
I've addressed the first part in 26fb606.
I'd prefer not to do the second part (moving the reading of the next left series to one place), as that will mean we're holding more series in memory for longer while we read the right series. It may also mean that downstream operators on the left side are holding more intermediate state while the right side is evaluated, which would again increase peak memory consumption.
There was a problem hiding this comment.
Actually, instead of reading it to just discard it, what do you think about extending InstantVectorOperator to have a SkipSeries method that moves the selector forward so we don't have to get slices for all the points etc.
There was a problem hiding this comment.
I had the same idea, I've already added something like this to our list of performance improvement ideas 🙂 I think this makes sense, but this is a far bigger change outside the scope of this PR (and I suspect outside the scope of our initial focus of getting MQE up and running).
9d42f1b to
26fb606
Compare
…similar issues in the future
Not dramatically - reusing the left series slice for data improved things by 1-2%, and reusing the metadata slice made no difference (within the margin of error of the benchmarks). |
What this PR does
This PR adds support for the
andbinary operation.Compared to Prometheus' engine, latency is up to 90% lower and peak memory consumption is up to 48% lower in our benchmarks:
Which issue(s) this PR fixes or relates to
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.