Revert "Combine small pages in Limit" #129107
Merged
Conversation
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
dnhatn
changed the title
Combine small pages in Limit
elasticsearchmachine
added
the
Team:Analytics
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
ivancea
approved these changes
Jun 9, 2025
nik9000
approved these changes
Jun 9, 2025
I wrote some stuff that uses those too! I should have thought about that. Good thing we have the benchmarks. |
|
Thanks for reviewing, Ivan and Nik! |
benchaplin
pushed a commit
to benchaplin/elasticsearch
that referenced
this pull request
Jun 9, 2025
This PR reverts elastic#128531. With elastic#128531, the Limit operator was updated to combine smaller pages into a larger page to reduce overhead, such as the number of exchange requests. However, this has a significant implication: the combined larger page does not retain the attributes of the blocks from the smaller pages. For example, if the smaller pages have ordinal-based BytesRef blocks, the larger page will not. This can cause a significant slowdown if subsequent operators have optimizations for ordinal-based blocks. The Enrich operator has such optimizations, and our benchmarks have shown this performance regression. One possible solution to reduce the regression is to set a threshold (e.g., 1000 rows), above which the Limit operator would pass the page along without combining. However, even with a threshold of 1000, the performance regression does not go away completely. Alternatively, we could allow exchange requests to return multiple pages (up to the page size limit). To minimize risk, this PR reverts the previous change, and we will reintroduce a new change later
valeriy42
pushed a commit
to valeriy42/elasticsearch
that referenced
this pull request
Jun 12, 2025
This PR reverts elastic#128531. With elastic#128531, the Limit operator was updated to combine smaller pages into a larger page to reduce overhead, such as the number of exchange requests. However, this has a significant implication: the combined larger page does not retain the attributes of the blocks from the smaller pages. For example, if the smaller pages have ordinal-based BytesRef blocks, the larger page will not. This can cause a significant slowdown if subsequent operators have optimizations for ordinal-based blocks. The Enrich operator has such optimizations, and our benchmarks have shown this performance regression. One possible solution to reduce the regression is to set a threshold (e.g., 1000 rows), above which the Limit operator would pass the page along without combining. However, even with a threshold of 1000, the performance regression does not go away completely. Alternatively, we could allow exchange requests to return multiple pages (up to the page size limit). To minimize risk, this PR reverts the previous change, and we will reintroduce a new change later
dnhatn
added a commit
that referenced
this pull request
Aug 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This PR reverts #128531.
With #128531, the Limit operator was updated to combine smaller pages into a larger page to reduce overhead, such as the number of exchange requests. However, this has a significant implication: the combined larger page does not retain the attributes of the blocks from the smaller pages. For example, if the smaller pages have ordinal-based BytesRef blocks, the larger page will not. This can cause a significant slowdown if subsequent operators have optimizations for ordinal-based blocks. The Enrich operator has such optimizations, and our benchmarks have shown this performance regression.
One possible solution to reduce the regression is to set a threshold (e.g., 1000 rows), above which the Limit operator would pass the page along without combining. However, even with a threshold of 1000, the performance regression does not go away completely. Alternatively, we could allow exchange requests to return multiple pages (up to the page size limit). To minimize risk, this PR reverts the previous change, and we will reintroduce a new change later