tomvanbussel commented on a change in pull request #29785:
URL: https://github.com/apache/spark/pull/29785#discussion_r490291817
##########
File path:
core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java
##########
@@ -398,11 +415,14 @@ public void insertRecord(
spill();
}
+ // Check if a new pointer array has to be allocated both before and after
a new page allocated,
+ // to make sure there is a pointer array even if spilling is triggered.
growPointerArrayIfNecessary();
int uaoSize = UnsafeAlignedOffset.getUaoSize();
// Need 4 or 8 bytes to store the record length.
final int required = length + uaoSize;
acquireNewPageIfNecessary(required);
+ growPointerArrayIfNecessary();
Review comment:
> `acquireNewPageIfNecessary` and `growPointerArrayIfNecessary` go hand
in hand, maybe we should combine them in a single method?
Added `allocateMemoryForRecordIfNecessary`.
> What happens if first `acquireNewPageIfNecessary` and then
`growPointerArrayIfNecessary` spills? Will there be a page we can write to?
That won't be an issue. `spill()` prevents memory from being freed if no
records have been inserted yet (which is the case after a spill). I added a
comment to clarify this but it is a bit wordy. Let me know if you see a way to
simplify the comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
