-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
ESQL: Page shouldn't close a block twice #100370
Changes from all commits
a7cb6ab
63de76d
7932c05
5cc7539
86ef454
e3415a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
pr: 100370 | ||
summary: "ESQL: Page shouldn't close a block twice" | ||
area: ES|QL | ||
type: bug | ||
issues: | ||
- 100356 | ||
- 100365 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ | |
|
||
import java.io.IOException; | ||
import java.util.Arrays; | ||
import java.util.Collections; | ||
import java.util.IdentityHashMap; | ||
import java.util.Objects; | ||
|
||
/** | ||
|
@@ -88,9 +90,7 @@ private Page(Page prev, Block[] toAdd) { | |
this.positionCount = prev.positionCount; | ||
|
||
this.blocks = Arrays.copyOf(prev.blocks, prev.blocks.length + toAdd.length); | ||
for (int i = 0; i < toAdd.length; i++) { | ||
this.blocks[prev.blocks.length + i] = toAdd[i]; | ||
} | ||
System.arraycopy(toAdd, 0, this.blocks, prev.blocks.length, toAdd.length); | ||
} | ||
|
||
public Page(StreamInput in) throws IOException { | ||
|
@@ -169,8 +169,8 @@ public Page appendPage(Page toAdd) { | |
@Override | ||
public int hashCode() { | ||
int result = Objects.hash(positionCount); | ||
for (int i = 0; i < blocks.length; i++) { | ||
result = 31 * result + Objects.hashCode(blocks[i]); | ||
for (Block block : blocks) { | ||
result = 31 * result + Objects.hashCode(block); | ||
} | ||
return result; | ||
} | ||
|
@@ -221,7 +221,48 @@ public void writeTo(StreamOutput out) throws IOException { | |
* Release all blocks in this page, decrementing any breakers accounting for these blocks. | ||
*/ | ||
public void releaseBlocks() { | ||
if (blocksReleased) { | ||
return; | ||
} | ||
|
||
blocksReleased = true; | ||
Releasables.closeExpectNoException(blocks); | ||
|
||
// blocks can be used as multiple columns | ||
var map = new IdentityHashMap<Block, Boolean>(mapSize(blocks.length)); | ||
for (Block b : blocks) { | ||
if (map.putIfAbsent(b, Boolean.TRUE) == null) { | ||
Releasables.closeExpectNoException(b); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
* Returns a Page from the given blocks and closes all blocks that are not included, from the current Page. | ||
* That is, allows clean-up of the current page _after_ external manipulation of the blocks. | ||
* The current page should no longer be used and be considered closed. | ||
*/ | ||
public Page newPageAndRelease(Block... keep) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've introduced this method for handling the case of reusing some blocks from an old page into a new one. |
||
if (blocksReleased) { | ||
throw new IllegalStateException("can't create new page from already released page"); | ||
} | ||
|
||
blocksReleased = true; | ||
|
||
var newPage = new Page(positionCount, keep); | ||
var set = Collections.newSetFromMap(new IdentityHashMap<Block, Boolean>(mapSize(keep.length))); | ||
set.addAll(Arrays.asList(keep)); | ||
|
||
// close blocks that have been left out | ||
for (Block b : blocks) { | ||
if (set.contains(b) == false) { | ||
Releasables.closeExpectNoException(b); | ||
} | ||
} | ||
|
||
return newPage; | ||
} | ||
|
||
static int mapSize(int expectedSize) { | ||
return expectedSize < 2 ? expectedSize + 1 : (int) (expectedSize / 0.75 + 1.0); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} |
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.
Small improvement.