[SPARK-10474] [SQL] Aggregation fails to allocate memory for pointer array (round 2)

[andrewor14]

This patch reverts most of the changes in a previous fix #8827.

The real cause of the issue is that in TungstenAggregate's prepare method we only reserve 1 page, but later when we switch to sort-based aggregation we try to acquire 1 page AND a pointer array. The longer-term fix should be to reserve also the pointer array, but for now _we will simply not track the pointer array_. (Note that elsewhere we already don't track the pointer array, e.g. here)

Note: This patch reuses the unit test added in #8827 so it doesn't show up in the diff.

[davies]

LGTM

[SparkQA]

Test build #42924 has finished for PR 8888 at commit c910d0b.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #42925 has finished for PR 8888 at commit ed36351.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1803 has started for PR 8888 at commit ed36351.

[SparkQA]

Test build #1804 has started for PR 8888 at commit ed36351.

[SparkQA]

Test build #1799 has finished for PR 8888 at commit ed36351.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1806 has started for PR 8888 at commit ed36351.

[SparkQA]

Test build #1805 has started for PR 8888 at commit ed36351.

[SparkQA]

Test build #1800 has finished for PR 8888 at commit ed36351.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1802 has finished for PR 8888 at commit ed36351.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1801 has finished for PR 8888 at commit ed36351.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[andrewor14]

By the way @JoshRosen this seems to run core tests like SecurityManagerSuite even though it touches only SQL stuff. Just FYI.

[chenghao-intel]

https://github.com/apache/spark/blob/master/core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java#L138 Seems reserve the data page only if the existingInMemorySorter is unset, however, SPARK-474 is not this case right? As it follows the sort merge join operator.

[JoshRosen]

@andrewor14, it's running core tests because you changed core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

[andrewor14]

@chenghao-intel SPARK-10474 is caused by an aggregate falling back to sort-based aggregation. In this case we don't acquire the page in the constructor, but we do acquire it when we insert into the sorter later.

[andrewor14]

it's running core tests because you changed core/src/main/java/org/apache/spark/util/collection/unsafe/sort/UnsafeExternalSorter.java

Ah yes never mind. I didn't realize it was in core.

[SparkQA]

Test build #1807 has started for PR 8888 at commit 2dc34c3.

[chenghao-intel]

@andrewor14 that's actually what I mean, if we didn't reserve the memory when creating the down streaming operator, we probably never get the chance to acquire the page when inserting records to sorter, as the upstreaming operator (like SMJ) probably eat out all of the memory, which even lead to the hash aggregation switching to sort-based aggregation when first record comes. Do you think that's possible?

[andrewor14]

@chenghao-intel but we do reserve the page in advance. See TungstenAggregate:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/TungstenAggregate.scala

Line 77 in 084e4e1

def preparePartition(): TungstenAggregationIterator = {

[SparkQA]

Test build #42942 has finished for PR 8888 at commit 2dc34c3.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[andrewor14]

OK it passed tests. I'm merging this into master 1.5.

[SparkQA]

Test build #1808 has finished for PR 8888 at commit 2dc34c3.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1811 has finished for PR 8888 at commit 2dc34c3.

• This patch fails Spark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1810 has finished for PR 8888 at commit 2dc34c3.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #1809 has finished for PR 8888 at commit 2dc34c3.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[chenghao-intel]

Thank you @andrewor14 for the explanation. I believe you're talking about to reserve the data page in advanced via the UnsafeFixedWidthAggregationMap.map (BytesToBytesMap), which will reserve the data page in its constructor.
Everything looks reasonable to me now, the only concern is the performance, since only a single data page available for the sort-based aggregation. Anyway, it's not related to this fix.

[andrewor14]

Yes, it's not related. What you mention here is a bigger problem. The current solution only ensures that we don't starve any operators. In the future we can improve this mechanism by introducing some force spilling mechanism, but that's too big of a change to backport to 1.5.