Mimir query engine: correctly handle functions that produce multiple series with the same labels #9019
Conversation
| @@ -30,6 +30,16 @@ func TestDropSeriesName(t *testing.T) { | |||
| require.Equal(t, expected, modifiedMetadata) | |||
| } | |||
|
|
|||
| func TestDropSeriesName_DuplicateSeries(t *testing.T) { | |||
| @@ -22,6 +24,11 @@ func DropSeriesName(seriesMetadata []types.SeriesMetadata, _ *limiting.MemoryCon | |||
| seriesMetadata[i].Labels = seriesMetadata[i].Labels.DropMetricName() | |||
| } | |||
|
|
|||
| if hasDuplicate, example := types.HasDuplicateSeries(seriesMetadata); hasDuplicate { | |||
There was a problem hiding this comment.
Should this check not be done in query.go when we get all of the series names?
It would help catch other operators incorrectly handling series labels such as aggregations. Although that's more likely a bug and unnecessary overhead. Or perhaps we do it in query.go in convertFunctionCallToOperator so that we apply it to all functions that can manipulate labels. Otherwise if we manipulate labels differently in a future function we may forget to apply this check.
There was a problem hiding this comment.
Should this check not be done in query.go when we get all of the series names? It would help catch other operators incorrectly handling series labels such as aggregations. Although that's more likely a bug and unnecessary overhead.
This wouldn't be a thorough-enough check - duplicate series are not permitted in any intermediate stage, not just the end result. And yep, I think it would be unnecessary overhead to do this and a bug if something was not correctly handling series labels.
Or perhaps we do it in query.go in
convertFunctionCallToOperatorso that we apply it to all functions that can manipulate labels. Otherwise if we manipulate labels differently in a future function we may forget to apply this check.
I'm thinking something more along these lines makes sense, especially given this realisation over the weekend.
Given we need similar checking and merging logic in many places1, I'm wondering if this should be done in an operator that wraps operators that manipulate labels. This wrapping operator would be responsible for:
- merging the results from different input series that map to the same output series
- rejecting queries where results from different input series conflict at a single timestamp
We can then apply this operator whenever the expression manipulates labels.
The other option would be to add this logic to FunctionOverInstantVector and FunctionOverRangeVector, and possibly other places in the future, but I think that will make them more complex to reason about and unnecessarily duplicate some logic that can live in one place.
What do you think?
Footnotes
-
all the functions over instant vectors and range vectors,
label_replace,label_join, unary negation, possibly others? ↩
There was a problem hiding this comment.
I think a wrapper operator makes the most sense. I think the only reason not to would be if we have an opportunity in a downstream operator to detect such a conflict. However, we can likely do both and if we have an opportunity for such an optimisation we can percolate the error back up.
|
Thinking about this more, I realised the solution isn't quite as simple as this, so I'm going to put this PR back to a draft until I can implement this properly: It's not enough to check for duplicate series, what we need to do is check for conflicting samples for the same output series, and merge the results from input series that map to the same output series. eg. imagine we're running the query If the input data is: then we should not produce an error and should merge the result to a single series: However, the implementation here will cause the query to return an error. And, without this PR, we'll currently return this result, which is also invalid: The only time we should return an error is if the input series map to the same output series and have a conflicting sample, for example: |
charleskorn
force-pushed
the
charleskorn/duplicate-series
branch
from
47fdcdf to
5220a74
Compare
…es returned by `HasDuplicateSeries`
charleskorn
changed the title
# Conflicts: # CHANGELOG.md # pkg/streamingpromql/operators/binary_operation_test.go
| @@ -37,7 +37,8 @@ type ScalarFunctionOperatorFactory func( | |||
| // - name: The name of the function | |||
| // - metadataFunc: The function for handling metadata | |||
| // - seriesDataFunc: The function to handle series data | |||
| func SingleInputVectorFunctionOperatorFactory(name string, metadataFunc functions.SeriesMetadataFunction, seriesDataFunc functions.InstantVectorFunction) InstantVectorFunctionOperatorFactory { | |||
| // - needsSeriesDeduplication: Set to true if metadataFunc may produce multiple series with the same labels and therefore deduplication is required | |||
There was a problem hiding this comment.
Note to reviewers: we've previously talked about extracting a type like FunctionOverRangeVector for functions over instant vectors, and I think the addition of this argument is another reason to do this, but I don't want to do this in this PR in the interests of keeping it to a manageable size.
|
Given the approval, I'm going to merge this, but happy to address any post-merge feedback in a follow-up PR. |
What this PR does
This PR fixes MQE's handling of queries that produce results with multiple series with the same labels.
This is currently only possible in MQE in places where we drop the metric name from the input series: in all other cases it's not possible to return multiple series with the same labels. (Functions like
label_replaceand unary negation will need similar checks when we implement those features.)See #9019 (comment) for more explanation of the logic in this 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.