MQE: add support for filtering in multi-aggregation nodes#14457
MQE: add support for filtering in multi-aggregation nodes#14457charleskorn merged 3 commits intomainfrom
Conversation
charleskorn
force-pushed
the
charleskorn/subset-selector-elimination-multi-aggregation
branch
from
66b2e02 to
973cee1
Compare
charleskorn
force-pushed
the
charleskorn/subset-selector-elimination-multi-aggregation
branch
from
973cee1 to
41ae315
Compare
charleskorn
force-pushed
the
charleskorn/subset-selector-elimination-multi-aggregation
branch
from
41ae315 to
38c5d3c
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
# Conflicts: # CHANGELOG.md
charleskorn
force-pushed
the
charleskorn/subset-selector-elimination-multi-aggregation
branch
from
38c5d3c to
57c612c
Compare
lamida
left a comment
lamida
left a comment
There was a problem hiding this comment.
I left some comments but more on clarifying things. I can approve once it is clarified.
|
|
||
| m.unfilteredSeriesBitmap, err = types.BoolSlicePool.Get(len(unfilteredSeries), m.group.memoryConsumptionTracker) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
In this case do we need to return types.SeriesMetadataSlicePool.Put?
There was a problem hiding this comment.
Our general approach elsewhere has been that we don't mind not returning things to pools if something goes wrong, so I don't think we should do something different here.
pkg/streamingpromql/optimize/plan/multiaggregation/optimization_pass_test.go
Show resolved
Hide resolved
| if filteringSupported { | ||
| // If the parent is a DuplicateFilter, we need to look at the grandparent. | ||
| if _, isDuplicateFilter := parent.(*commonsubexpressionelimination.DuplicateFilter); isDuplicateFilter { | ||
| parent = path[len(path)-2] |
There was a problem hiding this comment.
What prevent this going out of bound?
There was a problem hiding this comment.
If there is a duplicate expression, there must be at least one node higher in the path (ie. a parent) - it's not possible to construct a PromQL expression containing a duplicate subexpression where the duplicated expression doesn't have a parent of its own, such as a binary expression or function call.
charleskorn
deleted the
charleskorn/subset-selector-elimination-multi-aggregation
branch
2 participants
What this PR does
This PR is derived from #14377, and extends the existing multi-aggregation logic so that no buffering is required for expressions like
sum(request_count{status="success}) / sum(request_count).See #14377 for more context.
This PR builds on #14455 and #14456.
Which issue(s) this PR fixes or relates to
(none)
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.Note
Medium Risk
Changes MQE execution/planning for multi-aggregation to apply per-consumer label filters and introduces
QueryPlanV8, which could affect query correctness/performance and cross-version compatibility if any edge cases are missed.Overview
Enables multi-aggregation consumers to carry and apply selector filters, so expressions like
sum(foo{...}) / sum(foo)can share a single underlying scan without buffering.This extends the multi-aggregation plan/node protobuf (
MultiAggregationInstanceDetails) and materialization to pass matchers intoMultiAggregatorInstanceOperator, which now precomputes a per-series bitmap and skips accumulation for instances whose filters don’t match.The multi-aggregation optimization pass is updated to fold
AggregateExpressionoverDuplicateFilterinto filteredMultiAggregationInstancewhen the supported plan version is >=QueryPlanV8;MaximumSupportedQueryPlanVersionis bumped toV8, tests are expanded for subset-selector scenarios and version gating, and the changelog entry is updated.Written by Cursor Bugbot for commit 8c397b1. This will update automatically on new commits. Configure here.