perf: Avoid copying when materializing output in OrderedPartialAggregateStream - #25312
Open
2010YOUY01 wants to merge 3 commits into
Open
2010YOUY01 wants to merge 3 commits into
2010YOUY01 wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25312 +/- ##
==========================================
+ Coverage 81.88% 81.92% +0.03%
==========================================
Files 1133 1134 +1
Lines 424522 425899 +1377
Branches 424522 425899 +1377
==========================================
+ Hits 347623 348916 +1293
- Misses 56285 56303 +18
- Partials 20614 20680 +66 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
part of #25157
Rationale for this change
Cause
See issue for the target query.
The query plan looks like
Query plan, Click to expand
It's slow due to inefficient output materializing in partial and final aggregation
For internal mechanism, this comment explains 'why not X, and do Y instead' -- X is the existing impl, Y is what this PR does.
Fix
To fully restore the performance, we have to fix:
2 uses almost the same mechanism as 1, so once this PR is reviewed, we can apply the pattern mechanically.
After this PR, the query runs in: (on an M4 Pro MacBook Pro)
What changes are included in this PR?
Note to read this PR, I suggest directly reading the new impl start from the entry point of state machine (
into_stream()), instead of the diff, due to a large refactor.This refactor is necessary because its easier to implement this feature with a different state machine pattern.
What is the testing strategy for this PR?
For correctness, existing tests have covered it.
To prevent similar perf regression, we can do
Are there any user-facing changes?