ESQL: Copy blocks when expanding #100548
Closed
ESQL: Copy blocks when expanding #100548
Conversation
This file contains 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
Until we get lovely reference tracking, let's copy the blocks. This fixes the memory tracking around mv_expand, but it requires a copy. For now, that's simple and it works. I expect we won't copy forever. We can likely share the array once we get reference tracking. But for now, let's copy!
elasticsearchmachine
added
the
Team:QL (Deprecated)
|
Pinging @elastic/es-ql (Team:QL) |
|
Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL) |
|
I'm not sure if we'll need to do this with @luigidellaquila's fix to cut blocks into more than one page on mv_expand, but this works for now. Have a look and decide what to do with it. |
dnhatn
approved these changes
Oct 9, 2023
| boolean success = false; | ||
| try { | ||
| for (int b = 0; b < result.length; b++) { | ||
| result[b] = b == channel ? expandedBlock : page.getBlock(b).filter(duplicateFilter); |
There was a problem hiding this comment.
Should we assign the expandedBlock to the result array before the loop to ensure that we always release it in case we hit the breaker with b < channel ?
| $endif$ | ||
| } | ||
| } | ||
| return builder.mvOrdering(mvOrdering()).build(); |
There was a problem hiding this comment.
I think we can assign MvOrdering.DEDUPLICATED_AND_SORTED_ASCENDING for the expanded block?
There was a problem hiding this comment.
Ah! It shouldn't really matter because it doesn't have duplicated blocks, but that's technically correct!
23 tasks
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Oct 12, 2023
Until we get lovely reference tracking, let's copy blocks when "expanding" them. For now, that's simple and it works and helps make memory tracking cleaner. I expect we won't copy forever. We can likely share the array once we get reference tracking. But for now, let's copy! Closes elastic#100548
|
Replaced by #100778. |
elasticsearchmachine
pushed a commit
that referenced
this pull request
Oct 12, 2023
Until we get lovely reference tracking, let's copy blocks when "expanding" them. For now, that's simple and it works and helps make memory tracking cleaner. I expect we won't copy forever. We can likely share the array once we get reference tracking. But for now, let's copy! Closes #100548
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
Oct 12, 2023
Until we get lovely reference tracking, let's copy blocks when "expanding" them. For now, that's simple and it works and helps make memory tracking cleaner. I expect we won't copy forever. We can likely share the array once we get reference tracking. But for now, let's copy! Closes elastic#100548
elasticsearchmachine
pushed a commit
that referenced
this pull request
Oct 12, 2023
Until we get lovely reference tracking, let's copy blocks when "expanding" them. For now, that's simple and it works and helps make memory tracking cleaner. I expect we won't copy forever. We can likely share the array once we get reference tracking. But for now, let's copy! Closes #100548
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.
Until we get lovely reference tracking, let's copy the blocks. This fixes the memory tracking around mv_expand, but it requires a copy. For now, that's simple and it works. I expect we won't copy forever. We can likely share the array once we get reference tracking. But for now, let's copy!
Closes #100548