-
Notifications
You must be signed in to change notification settings - Fork 694
MQE: Eliminate deduplicate and merge nodes for binary expressions #13950
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
414c1eb to
fc4bc16
Compare
This change extends the `EliminateDeduplicateAndMergeOptimizationPass` to attempt to remove deduplicate and merge nodes on either side of a binary expression. Prerequisite for #13863 Signed-off-by: Nick Pillitteri <[email protected]>
fc4bc16 to
fadd1cc
Compare
charleskorn
left a comment
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.
Overall LGTM. Could you please add a test for cases where a binary operation is nested? (eg. foo or bar or baz, or rate(foo[1m]) or rate(bar[1m]) or rate(baz[1m]))
Signed-off-by: Nick Pillitteri <[email protected]>
Signed-off-by: Nick Pillitteri <[email protected]>
charleskorn
left a comment
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 modulo suggestions below
pkg/streamingpromql/optimize/plan/eliminate_deduplicate_and_merge.go
Outdated
Show resolved
Hide resolved
pkg/streamingpromql/optimize/plan/eliminate_deduplicate_and_merge_test.go
Outdated
Show resolved
Hide resolved
* use slices.ContainsFunc * assert metrics when queries are not modified Signed-off-by: Nick Pillitteri <[email protected]>
| } | ||
|
|
||
| // getSelectorType determines if node is a selector and whether it has an exact name matcher. | ||
| func getSelectorType(node planning.Node) SelectorType { |
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.
Label replace handling incorrectly marks sibling branch nodes
Medium Severity
The label_replace handling at lines 148-150 assumes nodes[len-2] is always an ancestor of the label_replace function. However, with the new binary expression traversal (lines 93-103), the nodes list can now contain DeduplicateAndMerge nodes from sibling branches. For a query like rate(foo[5m]) or label_replace(rate(bar[5m]), ...), when processing label_replace, nodes[len-2] incorrectly points to foo_rate's dedup node (from the LHS branch) rather than an ancestor of label_replace. This causes the LHS dedup node to be incorrectly retained when it should be eliminated (since foo has an exact name matcher). Before this PR, binary expressions caused early return so this code path was never exercised.
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.
This results in keeping an extra DeduplicateAndMerge node which is less efficient than dropping it but doesn't cause in incorrect results. I'll handle this in a follow up PR.
What this PR does
This change extends the
EliminateDeduplicateAndMergeOptimizationPassto attempt to remove deduplicate and merge nodes on either side of a binary expression.Which issue(s) this PR fixes or relates to
Prerequisite for #13863
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
Improves query plan optimization by pruning unnecessary
DeduplicateAndMergenodes around binary expressions, respecting delayed name removal semantics.EliminateDeduplicateAndMergeOptimizationPassto keep only the required dedupe node for binary ops and eliminate others when safecortex_mimir_query_engine_eliminate_dedupe_attempted_totalandcortex_mimir_query_engine_eliminate_dedupe_modified_total; increments on attempts/plan changesRegisterer; all call sites updatedtypes.HasDuplicateSeriesfor 2-series caseWritten by Cursor Bugbot for commit ae195fe. This will update automatically on new commits. Configure here.